You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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 the verifyGraph 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.
Without the strict matching the graph tests are mostly useless. The test test-graph.http.js had HTTPPARSER in the expected graph and was green even HTTPPARSER was never emitted.
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:
add a flag like isMandatory to the graph and verify only the presence of them instead all of them.
remove non use case relevant entries from the expected graph and concentrate on the essence instead the details (e.g. in case of HTTP keep TCP/HTTP stuff but remove at least optional timeouts)
The approach of listing isMandatory or isRequired sounds reasonable to me. I'll try to open a PR for this the next time I get some time at the keyboard, but others are welcome to open a PR too.
Activity
Flarna commentedon May 9, 2019
Without the strict matching the graph tests are mostly useless. The test test-graph.http.js had
HTTPPARSERin the expected graph and was green evenHTTPPARSERwas never emitted.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:
isMandatoryto the graph and verify only the presence of them instead all of them.ofrobots commentedon May 13, 2019
The approach of listing
isMandatoryorisRequiredsounds reasonable to me. I'll try to open a PR for this the next time I get some time at the keyboard, but others are welcome to open a PR too.test: relax check in verify-graph
Flarna commentedon May 16, 2019
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
test: relax check in verify-graph
test: relax check in verify-graph