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

Conversation

Projects
None yet
2 participants
@r0mflip
Copy link

commented Apr 29, 2019

Tries to solve #465

Problem

The stream created inside results was being closed only when the process was exiting. Changed the API to introduce opts.autoclose that is provisioned over canEmitExit so that results get closed when the results are done. In normal usage(from command line) harness closes on process end.

autoclose is required beacause throughs queue method is storing the results untill the end where it is ended only when harness is closed.

Normal usage

const tapSpec = require('tap-spec');
const {add, divide} = require('../index.js');
const test = require('./tape/index.js');

// piping programatically
test.createStream()
  .pipe(tapSpec())
  .pipe(process.stdout);

test('should add', t => {
  t.equal(add(3, 4), 7);
  t.end();
});

test('should divide', t => {
  t.equal(divide(8, 2), 4);
  t.end();
});

we get the output:

> node test/test.js

  should add

    √ should be equal

  should divide

    √ should be equal

opts.autoclose set to true

test.createStream({
  autoclose: true,
})
  .pipe(tapSpec())
  .pipe(process.stdout);

we get

> node test/test.js

  should add

    √ should be equal

  should divide

    √ should be equal

  total:     2
  passing:   2
  duration:  43ms
@r0mflip

This comment has been minimized.

Copy link
Author

commented May 8, 2019

/pinging @ljharb @substack

@ljharb
Copy link
Collaborator

left a comment

Please add test cases that covers this.

index.js Outdated
@@ -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

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

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
@r0mflip

r0mflip May 11, 2019

Author

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

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

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

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!

Check `opts.autoclose` type
Co-Authored-By: Jordan Harband <ljharb@gmail.com>
@ljharb

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2019

Closing per #466 (comment), but happy to reopen if there's still a need!

@ljharb ljharb closed this May 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.