Open chrisnicola opened 7 years ago
I would expect that when a test errors because it "exited without ending", that the test would be marked as ended, and that any successive t.end()
calls are no-ops. This seems like a bug to me.
@ljharb thanks. I'll see if I can trace it to the cause today then.
So the problem seems to happen here during process.on('exit') which is being fired after the first test fails to call .end
. It then loops through the other tests calling _exit()
on each one without running them so they also report that the test exited without ending.
What I'm not clear on is why the process is exiting after the first failure.
Ok and this code is what causes the test suite to exit. It sets up assuming that an 'end' may be called in some asynchronous code, however if that isn't the case there is nothing left for node to do so the process exits. Then that exit code runs and called _exit
on the remaining tests.
A simple solution I tested initially was to a reasonable default timeout for all tests. It would seem that there is really no other way to not have undeterminable behaviour for synchronous tests that fail to call .end()
. However, this runs into a new problem which is that all of the other tests get held up when the while loop exits and waits for end
and they all fail and timeout as well.
I'm probably stabbing in the dark a bit at this point as I don't know this code that well but that's as far as I've got in the investigation so far.
Hmm the problem with the default timeout is stranger than I would have expected. It also causes a timeout error to occur for all the tests that have already run. I've gone back and verified that these have all run clearTimeout
and end
was called, so this is really strange to me.
Hey @ljharb sorry for the spam. I found the cause of the timeout issue. It appears that much like safeSetTimeout
the clearTimeout
call needs to be assigned to a local variable as well or it does not work as expected and the timeout is never actually cleared.
I'm not really sure this is the right solution for this library but I'll submit a PR anyways with what I think makes sense which is.
clearTimeout
that was preventing timeouts from successfully being cleared.So I've limited the change to just setting the clearTimeout
global as a local. I'm not sure why I needed that, but it may have something to do with the "webpacked" output in this case.
I do think there is a larger issue with how the end of a test is detected when end
is missing or the plan is incomplete. Moving to the assumption of a default timeout seems reasonable but it is a pretty big change from the current behaviour of tape right now so I don't want to make that change without further discussion.
Thanks for all the research! I'll take a look at the PR.
I'm running into the same issue. Here is a minimal test to reproduce:
const test = require('tape')
test('no end', (t) => {
t.ok(true)
})
test('second', (t) => {
t.ok(true)
t.end()
})
This fails twice instead of once.
I'm not yet able to provide much analysis for this issue, but I'll describe the setup and the behaviour as well as I can.
Setup
Using webpack to process the files before running the tests. These are Vue tests using single-file-components, so unfortunately Webpack is a requirement at the moment. I suspect that the output format of the webpack'd test files is a possible cause of this issue.
The output from webpack is then executed using node and the tests run fine. Using the sourcemaps plugin source locations work as well for most errors. However when there is a missing
t.end()
I get a repeating error for all of the remaining tests.Results
The output is below. Basically after it hits the test that fails to end it continues and reports the same error for all the following tests.
The stack trace is also a bit odd, in that it doesn't show the location of the test that failed to end.