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

Cannot create a stream then load ESM test modules #575

Open
ydarma opened this issue May 25, 2022 · 9 comments · May be fixed by #576
Open

Cannot create a stream then load ESM test modules #575

ydarma opened this issue May 25, 2022 · 9 comments · May be fixed by #576

Comments

@ydarma
Copy link
Contributor

ydarma commented May 25, 2022

Hi, the following won't work :

import test from 'tape';

test.createStream({ objectMode: true }).on('data', function (row) {
    console.log(JSON.stringify(row));
});

import('./testESMModule.mjs');
/* ./testESMModule.mjs */

import test from 'tape';

test('This one should pass', function (t) {
    t.pass();
    t.end();
});

Actually the test in the ESM Module does not run because the stream creates a result queue that is exhausted in the same event loop it is created in (results are consumed in a setImmediate callback). The ESM module will be loaded asynchronously, in a subsequent event loop.

The wait and run function used in the cli are not helpful here because the stream will be recreated on the run method, thus :

  • the stream will not be the one with registered listeners.
  • the tests are not registered on the stream because the run method is called after the test are defined in the test module.

For now I am using this trick :

import test from 'tape';

test.createStream({ objectMode: true }).on('data', function (row) {
    console.log(JSON.stringify(row));
});

test('__load_async__', async function (t) {
    await import('./testESMModule.mjs');
    t.end();
});

The result processing starts by the synchronously defined test named '__load_async__' in the same event loop in which the stream is created. It will then "lock" the stream until all modules are loaded (because t.end() is called once they're all loaded). Then the results processing continues with the asynchronously defined tests.

I propose something better, which implementation uses the same kind of mechanism as above :

import test from 'tape';

test.createStream({ objectMode: true }).on('data', function (row) {
    console.log(JSON.stringify(row));
});

test.async(async function () {
    await import('./testESMModule.mjs');
});

I implemented this here : https://github.com/ydarma/tape/blob/async-load/test/async-test-load.js (I can make a PR).

@ljharb
Copy link
Collaborator

ljharb commented May 25, 2022

I'm a bit confused.

test.async seems like something we shouldn't need. Tests that are async return a Promise from their callback, and tape knows what to do with that.

It makes sense to me that your OP won't work, because of the nature of ESM. I'd expect you to have to await import('./testESMModule.mjs'); at the top-level - does that do it?

@ydarma
Copy link
Contributor Author

ydarma commented May 25, 2022

No await import('./testESMModule.mjs); has no meaning at the top level (await is only allowed in an async function). Node runtime will not terminate until all promises is either resolved or rejected.

You're right tape manages promises well in tests - I have a lot in my own. The problem is for writing a test runner like https://github.com/substack/tape#object-stream-reporter - it won't work with ESM modules. This comes form the implementation of Results.prototype.createStream in file lib/results.js file, lines 85 to 95 :

        nextTick(function next() {
            var t;
            while (t = getNextTest(self)) {
                t.run();
                if (!t.ended) {
                    t.once('end', function () { nextTick(next); });
                    return;
                }
            }
            self.emit('done');
        });

The nextTick (actually setImmediate) will trigger before the ESM module is loaded (because it is asynchronous and will be done in a subsequent event loop). At this time no test is registered and the program jumps to line 94 and emits a done event.

I should have called the method test.loadAsync because it is not about testing asynchronous behaviors but about loading test modules in an asynchronous way. It's only relevant when a result stream is created before the test are loaded, otherwise it will be created later.

Just try : https://github.com/ydarma/tape/blob/async-load/asyncESMload.mjs, you'll see that it does not work.

We need our own test runner with an object stream because we record test results in a database. We recently migrated our code to ESM and ran into this problem with tape.

@ljharb
Copy link
Collaborator

ljharb commented May 25, 2022

no, await is permitted at the top-level in Modules, it's a language change. Try it.

@ydarma
Copy link
Contributor Author

ydarma commented May 25, 2022

Ah you're right await is now allowed - thanks for that. But this does not change the problem. The stream is exhausted before test are registered.

@ljharb
Copy link
Collaborator

ljharb commented May 25, 2022

ok, thanks for confirming.

I wonder if this is just a limitation of streams themselves?

Another alternative is some kind of opt-in way to ask the stream to "wait" until a promise resolves?

@ydarma
Copy link
Contributor Author

ydarma commented May 26, 2022

It is not a limitation of node streams. Really it comes from the tape implementation which does not wait enough before processing the tests and closes the stream early. The trick I found or the solution I proposed are exactly what you wrote above : waiting until promise resolve.

You should try the code on my fork : https://github.com/ydarma/tape/blob/async-load/asyncESMload.mjs with three breakpoints :

  1. in lib/results.js line 94 (when tape ends the result stream)
  2. in ./testESMModule.mjs line 3 (when the test is defined)
  3. in ./testESMModule.mjs line 4 (when the test is excuted)

I am pretty sure that the right hit order would be 2. -> 3. -> 1. but it is 1. -> 2. ; 3. is never hit.

Using the trick of encapsulating the module load in a synchronous test, the order is correct. As you wrote above, the synchronous test forces the stream processing (in function createStream) to wait until the promise is resolved.

Really the problem is that tape processes the stream in a setImmediate callback, i.e. in the same event loop as the createStream call. The module will be loaded later.

The problem is not new ; look at bin/tape, lines 85 to 93 :

    tape.wait();

    var filesPromise = files.reduce(function (promise, file) {
        return promise ? promise.then(function () {
            return importOrRequire(file);
        }) : importOrRequire(file);
    }, null);

    return filesPromise ? filesPromise.then(function () { tape.run(); }) : tape.run();

There is those tape.wait() and tape.run() calls that tell tape to start test processing after all modules are loaded. The Results.prototype.createStream is called by tape.run(). But it does not work anymore with a custom reporter because createStream must be called earlier.

In my proposal this code would become :

    return tape.async(function () {

        return files.reduce(function (promise, file) {
            return promise.then(function () {
                return importOrRequire(file);
            });
        }, Promise.resolve());

    });

Which I find not bad IMHO...

@ljharb
Copy link
Collaborator

ljharb commented May 26, 2022

I really don't like the tape.async approach.

So let's see if I understand. You're saying that bin/tape solves this problem by waiting until all of the files (passed in via CLI arg) have resolved, which includes any TLA in ESM files, before running tape.run() - but createStream doesn't have a solution for this available.

What happens if you call tape.wait() before tape.createStream(), and then call tape.run() after the dynamic import is complete?

@ydarma
Copy link
Contributor Author

ydarma commented May 27, 2022

I tried this too and is does not work. Results.prototype.createStream is called by tape.run(), thus after the tests are defined in the imported module. Consequently all event listeners that should be registered against the Test object (line 47 to 75 in Results.prototype.createStream) are not registered.

Results.prototype.createStream must be called before the tests are defined. When tape.run() is called it is too late.

If you prefer a behavior mutating style, it can be solved by the same kind of test processing lock I have implemented in tape.async() :

function createExitHarness(conf, wait) {
//...
    run();

    if (wait) {
        var waiter =  new EventEmitter();   // create a lock for the test processing stream
        waiter.run = function() {};
        harness._results.push(waiter);  // push the lock as the first thing to run in the processing stream
        harness.run = function() {
            waiter.emit("end");  // release the lock, allowing subsequent tests to run
        }
    }

I think that the waiter only needs the run() function from Test interface (in addition to EventEmitter) because other methods are assertion methods. Of course this implementation could be cleaned, but this is the idea.
I did this on my fork along with a passing unit test : https://github.com/ydarma/tape/blob/async-load/test/wait-run-object-stream.js

I do not know if it breaks something, I can't find other tests invloving wait and run calls. You are probably testing the cli separately.

@ydarma ydarma linked a pull request May 27, 2022 that will close this issue
@ydarma
Copy link
Contributor Author

ydarma commented May 27, 2022

I have pushed a PR #576 you will choose what you want to do. For now, I will continue to use a synchronous test to lock the processing stream, personally I prefer that to the mutating style wait() + run().
Ask if I can help, e.g. creating a Lock object that inherits EventEmitter and add a run method to its prototype...
Thanks

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

Successfully merging a pull request may close this issue.

2 participants