tape-testing / tape

tap-producing test harness for node and browsers
MIT License
5.77k stars 307 forks source link

`.only()` fails with asynchronous tests #585

Open finetjul opened 1 year ago

finetjul commented 1 year ago

If tests are added with <script> (which is the case with karma), the nextTick() callback may be processed before loading a test that has the .only property. In such case, the first tests will be executed.

E.g.

<script type="text/javascript" src="test1.js" ></script>
<script type="text/javascript" src="test2.js" ></script>
<script type="text/javascript" src="test3.js" ></script>
...
<script type="text/javascript" src="test23.js" ></script>
// at this time, maybe JRE is processing the timeouts, hence first tests are ran
<script type="text/javascript" src="test24.js" ></script>
<script type="text/javascript" src="test25.js" ></script>
...
<script type="text/javascript" src="test53.js" ></script>
<script type="text/javascript" src="testwith.only().js" ></script> // by the time, this script is parsed, many tests have already been executed
...
<script type="module">window.__karma__.loaded()</script>

Adding defer or async does not change anything.

ljharb commented 1 year ago

It's a bit tough to reproduce from here without the content of the scripts.

ljharb commented 1 year ago

Generally though, I'd expect browser tests to be always bundled into a single file, not directly loaded with a script tag?

finetjul commented 1 year ago

Here is my setup: karma.conf.js: image

The executed html file: image

A test file: image

The test with .only(): image

finetjul commented 1 year ago

Generally though, I'd expect browser tests to be always bundled into a single file, not directly loaded with a script tag?

I can't bundle all my test files into a single file, I'm using karma. I can't concatenate files into one (I considered karma-concat-preprocessor because of duplicated import statements (and that probably won't work well with sourcemap).

I tried to do import test from 'tape[-catch]'; test.wait(); and run the tests manually at the end via test.run(). Problem is that tests were not registered to the stream (.on('_push') in results.js was not called each time a test is added)

The only way I found to prevent the tests from running (i.e. setTimeout() from being called), is to replace setTimeout() with a custom window.setImmediate do before any test <script> :

let testRunner;
window.setImmediate = (runTest) => {
  testRunner = runTest;
};

And catch when karma is started as such:

function createStartFn(tc) {
  return function() {
    if (testRunner) {
      testRunner();
    } else {
      console.log('no test runner !');
    }
  };
}

window.__karma__.start = createStartFn(window.__karma__);

This is definitely not pretty, but that works. If you can find a better to do it, I take it :-)

ljharb commented 1 year ago

I'm not clear on why using karma precludes bundling; i've set up karma with a bundle before.

I'm not familiar with tape-catch - but it seems like it's possible that it was last updated when tape was in v3, and thus breaking changes in v4 and v5 may have caused it not to work properly anymore. It's ofc also possible that tape-catch is working fine :-)

I'm happy to try to change something in tape if that would help your use case, I'm just not clear on what that would be :-/

finetjul commented 1 year ago

I'm not clear on why using karma precludes bundling; i've set up karma with a bundle before.

Maybe it's doable, I just couldn't figure out. But I think it is "better" to have tape support being ran when tests are not bundled.

I'm not familiar with tape-catch - but it seems like it's possible that it was last updated when tape was in v3, and thus breaking changes in v4 and v5 may have caused it not to work properly anymore. It's ofc also possible that tape-catch is working fine :-)

I've the same problem without tape catch.

I'm happy to try to change something in tape if that would help your use case, I'm just not clear on what that would be :-/

The problem is that the setTimeout() next callback (in results.js) is called too early. The fix would be to give tape users the control on when the next function is being called. In a way, if you could 1) expose an option in createStream() that prevents setTimeout() from being called, 2) expose a function to manually call the next function, that would be great.

finetjul commented 1 year ago

Or more simply, there is a wait() function in tape. It would be sufficient if there is a resume() function as well.

ljharb commented 1 year ago

Isn't that .run()?

finetjul commented 1 year ago

Isn't that .run()?

I wish but it does not work. Here is what's going on:

test.wait();
test.createStream({objectMode:true}); // will not call Results.createStream() because of `.wait()`
...
test('Test1', (t) => {...}); // does not call `self.on("_push", ontest(...)` because `Results.createStream()` has not yet been called
...
test.run(); // will call `Results.createStream()` and will execute the tests but their events (e.g. "data") are not caught/forwarded to the stream.
ljharb commented 1 year ago

Hm, that seems like a bug with the “wait” functionality (cc @lohfu)

lkmill commented 1 year ago

i'll try to find some time this week to have a look!

finetjul commented 1 year ago

@lohfu have you had a chance to have a look ?

lkmill commented 1 year ago

@finetjul hi, sorry for the delay. unfortunately i'm an incredibly unreliable individual often falling into deep holes of darkness where i forget about the world around me and any commitments i have there.

i've had a quick look at your problem now, and while it's near impossible without substantial work to reproduce your issue from the screenshots you have provided i think solving it could require even more quirky workarounds than my .wait() and .run() functions without exposing more of the internals of harness creations than today. It was a a year or two ago since i worked on the esm conversion, but i recall wanting to expose createExitHarness instead of just createHarness. I cant remember exactly why I wanted to do this, but I struggle a little to see how the examples from your screenshots triggers the .wait() logic from the bin/tape executable.

I find it slightly odd you provide screenshots of code instead of actual pasted code. I would like to try to reproduce your issue locally, do you think there is any chance to you could provide a simple repo or gist of a similar setup to help me see what's going on?