-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
test: simplify test-async-hooks-http-parser-destroy #26963
Conversation
@Trott Sadly, an error occurred when I tried to trigger a build. :( |
It would be nice if the assert output for sets could be improved. For example, in the quoted example 159, 162, 165, 168 are listed in both |
@richardlau there is an open issue about this and it is on my TODO list. It is sadly just not an easy task. I need meta information about the structure of the inspected object. Otherwise it's not possible to reliable compare the correct objects. As soon as I have that, I would also like to implement a proper diffing algorithm and I already checked a couple of papers and some code about that as well. |
The good news is this version of the test passes on Pi now. The bad news is it fails when run inside a worker. Investigating. |
68da51f
to
b340477
Compare
At this point, I have to believe this is uncovering a real bug involving |
b340477
to
69cdc43
Compare
Rebased, force-pushed, no longer failing in a worker, but still seems to be flaky due to #26961 or related. (I added code to work around asyncIds being reused, but it appears that they also might not appear at all in the destroy handler, which causes the test to timeout.) |
Simplify test-async-hooks-http-parser-destroy and improve it's reporting when failing. (Also makes it easier to run on older versions of Node.js because it doesn't rely on internal test modules that won't work there.) Before, failures looked like this (edited slightly to conform to our commit message 72-char line length restriction): The expression evaluated to a falsy value: assert.ok(destroyedIds.indexOf(createdAsyncId) >= 0) Now, you get a slightly better idea of what's up. (Is it missing one ID? More than one? All of them?): Input A expected to strictly deep-equal input B: + expected - actual ... Lines skipped Set { 1009, ... 1025, - 158, - 159, - 161, - 162, - 164, - 165, - 167, - 168, - 170, ... + 159, + 162, + 165, + 168, + 171, + 174, + 177, + 180, + 183, This test still fails as expected on 10.14.1 and passees on 10.14.2 (where the regression it tests for was fixed). (You will need to comment on the `require('../common');` line first but that's now the only change you need to make this run in older versions.) Refs: nodejs#26610
69cdc43
to
079d054
Compare
Simplify test-async-hooks-http-parser-destroy and improve it's reporting
when failing. (Also makes it easier to run on older versions of Node.js
because it doesn't rely on internal test modules that won't work there.)
Before, failures looked like this (edited slightly to conform to our
commit message 72-char line length restriction):
Now, you get a slightly better idea of what's up. (Is it missing one ID?
More than one? All of them?):
This test still fails as expected on 10.14.1 and passees on 10.14.2
(where the regression it tests for was fixed). (You will need to comment
on the
require('../common');
line first but that's now the only changeyou need to make this run in older versions.)
Refs: #26610
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes