-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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/test-graph.http.js flaky #27617
Comments
Without the strict matching the graph tests are mostly useless. The test test-graph.http.js had If we remove the strict check I added in #27477 we should add something else instead in all the graph tests to verify that the relevant (?) resources are present. I can think about following options:
|
The approach of listing |
Relax the check regarding presence of async resources in graph to allow extra events. Before this change events not mentioned in reference graph were allowed but that one specified must match exactly in count. Now it's allowed to have more events of this type. Refs: nodejs#27477 Fixes: nodejs#27617
I found an even easier way to fix this, just allow extra events similar as we allow any event not mentioned at all in the reference graph. see #27742 |
Relax the check regarding presence of async resources in graph to allow extra events. Before this change events not mentioned in reference graph were allowed but that one specified must match exactly in count. Now it's allowed to have more events of this type. Refs: #27477 Fixes: #27617 PR-URL: #27742 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Background: #27558 (comment)
Async Hooks tests that verify that async resource graph – specifically
test/async-hooks/test-graph.http.js
– are being flaky with a low probability. We suspect this is due to theverifyGraph
improvements in #27477.Async Hooks exposes a lot of internal details, and in this case it ends being sensitive to the fact that for optimization and implementation details reasons we may occasionally (1 in 1000 in this case) create an async resource graph that the test originally expected.
IMO, we shouldn't have strict expectations on the structure of the async resource graph – there is a lot of implementation detail that is captured hence. The stricter check for expected types added as part of #27477 may end up being more problematic in the long term, IMO. At the least we should default to non-strict matching and use strict matching where the test-cases explicitly calls for it.
/cc @Flarna thoughts?
The text was updated successfully, but these errors were encountered: