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

.only() fails with asynchronous tests #585

Open
finetjul opened this issue Nov 16, 2022 · 13 comments
Open

.only() fails with asynchronous tests #585

finetjul opened this issue Nov 16, 2022 · 13 comments

Comments

@finetjul
Copy link

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
Copy link
Collaborator

ljharb commented Nov 16, 2022

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

@ljharb
Copy link
Collaborator

ljharb commented Nov 16, 2022

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

@finetjul
Copy link
Author

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

The executed html file:
image

A test file:
image

The test with .only():
image

@finetjul
Copy link
Author

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
Copy link
Collaborator

ljharb commented Nov 18, 2022

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
Copy link
Author

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
Copy link
Author

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

@ljharb
Copy link
Collaborator

ljharb commented Nov 19, 2022

Isn't that .run()?

@finetjul
Copy link
Author

finetjul commented Nov 19, 2022

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
Copy link
Collaborator

ljharb commented Nov 19, 2022

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

@lohfu
Copy link
Contributor

lohfu commented Nov 20, 2022

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

@finetjul
Copy link
Author

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

@lohfu
Copy link
Contributor

lohfu commented Nov 30, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants