-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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,async_hooks: stabilize tests on Windows #13381
Conversation
@@ -1,6 +1,11 @@ | |||
'use strict'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit curious about the name of this file... test-graph.signal.js
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the async hooks can be represented as a graph/tree (see https://dprof.js.org). The test-graph.*.js
files check that the graph is correct. The test-graph.signal.js
file check that the graph regarding signals is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I get the purpose of the test, just not why it doesn't follow the naming pattern of other tests (specifically, why test-graph.signal
instead of test-graph-signal
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is any reason, we can rename.
@refack Thanks for looking into these. Unfortunately I can't really help much, but I can give some thoughts and guesses, hopefully, it helps.
To summarize, I don't think there are any actual bugs the tests are just not robust enough. |
Post #13336 run:
|
52583d0
to
b0f0119
Compare
"Force" CI on Windows: https://ci.nodejs.org/job/node-stress-single-test/1291/nodes=win10/ |
@DavidCai1993 @nodejs/async_hooks PTAL at b0f0119 |
5d009c8
to
f83c32d
Compare
reported flake #13603 |
@refack Could you rebase this? Then I will take a closer look. |
f83c32d
to
8feb68c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, assuming the CI is green on all platforms.
Thanks for taking care of this, it is was a bit embarrassing that we didn't have tests enabled for windows.
Arm fail is known: #14015 |
PR-URL: nodejs#13381 Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
PR-URL: nodejs#13381 Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
8feb68c
to
7022260
Compare
PR-URL: nodejs#13381 Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
PR-URL: nodejs#13381 Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
PR-URL: #13381 Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
PR-URL: #13381 Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
PR-URL: #13381 Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
PR-URL: #13381 Reviewed-By: Andreas Madsen <amwebdk@gmail.com>
new
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
test,async_hooks