Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add `opts.autoclose` in `test.createStream()` #465 #466

Closed
wants to merge 2 commits into from
Closed
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

Next

Provision `opts.autoclose` even when process can emit exit

  • Loading branch information...
r0mflip committed Apr 29, 2019
commit c975533085bc46b1b6ee0110d41dbb8710993b2a
@@ -30,7 +30,7 @@ exports = module.exports = (function () {
if (!opts) opts = {};
if (!harness) {
var output = through();
getHarness({ stream: output, objectMode: opts.objectMode });
getHarness({ stream: output, objectMode: opts.objectMode, autoclose: opts.autoclose });
return output;
}
return harness.createStream(opts);
@@ -50,7 +50,7 @@ exports = module.exports = (function () {

function getHarness(opts) {
if (!opts) opts = {};
opts.autoclose = !canEmitExit;
opts.autoclose = opts.autoclose || !canEmitExit;

This comment has been minimized.

Copy link
@ljharb

ljharb May 8, 2019

Collaborator

so if autoclose is true, this will be true, but if it's false, this won't be guaranteed to be false.

should this instead be?:

Suggested change
opts.autoclose = opts.autoclose || !canEmitExit;
opts.autoclose = typeof opts.autoclose === 'boolean' ? opts.autoclose : !canEmitExit;

?
or instead should it be:

Suggested change
opts.autoclose = opts.autoclose || !canEmitExit;
opts.autoclose = canEmitExit ? opts.autoclose : false;

This comment has been minimized.

Copy link
@r0mflip

r0mflip May 10, 2019

Author Contributor

canEmitExit is based on the environment, but opts.autoclose is meant to close the stream before exit whatever the environment be. (The first suggestion is what I'am going for)

What are your thoughts on canEmitExit? Should we leave it alone or close the stream once the tests are done whatever the environment maybe?

This comment has been minimized.

Copy link
@ljharb

ljharb May 10, 2019

Collaborator

I'm a bit unclear still, streams aren't where my knowledge lies :-) can you explain a bit more?

This comment has been minimized.

Copy link
@r0mflip

r0mflip May 11, 2019

Author Contributor

canEmitExit is used to know if tape is running in browser or in a process like node. If inside a browser the stream that spits out the results/output is closed when the "results end", this is done when the done event is fired on the results.

But in environments like node the steam is closed when the process ends. This is to facilitate the use of tape test/*.js usage (what I think is). But when this is used the last 3 lines for the output (the one which shows the aggregated resuls) which I was trying won't be spit out when I use tape as a module. When run through tape test/*.js the stream spits those 3 lines to stdout, thus the output is preserved. But when required as a module and a stream is created manually, the output is contained inside the process, when it ends the output is gone because the stream is just a variable that died without spitting it to stdout. This is delayed by the resumer package. It waits to put the chunks out onto the target stream.

My actual suggestion is to actually remove canEmitExit totally and to just close the stream right away. But I think it might cause other problems.

This comment has been minimized.

Copy link
@ljharb

ljharb May 11, 2019

Collaborator

What other problems would it cause?

This comment has been minimized.

Copy link
@ljharb

ljharb May 11, 2019

Collaborator

Are there circumstances where someone might want the process to exit immediately (initiated by user code) and tape would still need to suppress the final output?

This comment has been minimized.

Copy link
@r0mflip

r0mflip May 11, 2019

Author Contributor

What other problems would it cause?

I don't think it would cause any

Are there circumstances where someone might want the process to exit immediately (initiated by user code) and tape would still need to suppress the final output?

No, I don't think so, if the process exits normally (user initiated) then all the other tests are reported as "exited without ending", when it ends abnormally (user initiated) then nothing extra is printed, the whole process just quits. In both cases the final output is not shown 😟

This comment has been minimized.

Copy link
@ljharb

ljharb May 11, 2019

Collaborator

If no tests break, and there's no obvious problems caused by removing canEmitExit, we might as well remove it?

This comment has been minimized.

Copy link
@r0mflip

r0mflip May 11, 2019

Author Contributor

Yes, and also I've spotted a few unused variables lying around that look like they have a purpose but are not used at all. I wonder, why actually they were written?

This comment has been minimized.

Copy link
@ljharb

ljharb May 11, 2019

Collaborator

¯\_(ツ)_/¯ it predates my involvement in the project.

This comment has been minimized.

Copy link
@r0mflip

r0mflip May 11, 2019

Author Contributor

I'll see what I can do. I'll try to remove canEmitExit, and test it, Add tests for the current situation and remove the opts.autoclose

This comment has been minimized.

Copy link
@r0mflip

r0mflip May 12, 2019

Author Contributor

I had a good sleep and after looking around, it looks like I was wrong. canEmitExit is there so that the process exists with the correct exit code. Looks like we need the explicit autoclose for that one specific purpose.

This comment has been minimized.

Copy link
@r0mflip

r0mflip May 12, 2019

Author Contributor

Reporters like tap-mocha-reporter work fine without autoclose, it consumes the stream quickly and directly spits it to stdout only tap-spec has this problem because it uses it's own tap-out parser which tries to read the stream in a different way.

This comment has been minimized.

Copy link
@r0mflip

r0mflip May 12, 2019

Author Contributor

Alas!! I've found a way where tap-out could work just like tap-mocha-reporter. We don't need to implement autoclose anymore, it depends on the reporter now.

We can ignore this PR now I think 😀

This comment has been minimized.

Copy link
@ljharb

ljharb May 14, 2019

Collaborator

Thanks for the update!

if (!harness) harness = createExitHarness(opts);
return harness;
}
@@ -106,7 +106,7 @@ function createHarness(conf_) {
if (!conf_) conf_ = {};
var results = createResult();
if (conf_.autoclose !== false) {
results.once('done', function () { results.close() });
results.once('done', function () { if (!results.closed) results.close() });
}

var test = function (name, conf, cb) {
@@ -151,7 +151,7 @@ function createHarness(conf_) {
};
test._exitCode = 0;

test.close = function () { results.close() };
test.close = function () { if (!results.closed) results.close() };

return test;
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.