Skip to content
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

Closed
ofrobots opened this issue May 8, 2019 · 3 comments
Closed

test/async-hooks/test-graph.http.js flaky #27617

ofrobots opened this issue May 8, 2019 · 3 comments
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI.

Comments

@ofrobots
Copy link
Contributor

ofrobots commented May 8, 2019

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 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.

/cc @Flarna thoughts?

@Flarna
Copy link
Member

Flarna commented May 9, 2019

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)

@ChALkeR ChALkeR added the flaky-test Issues and PRs related to the tests with unstable failures on the CI. label May 10, 2019
@ofrobots
Copy link
Contributor Author

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.

Flarna added a commit to dynatrace-oss-contrib/node that referenced this issue May 16, 2019
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
@Flarna
Copy link
Member

Flarna commented 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

targos pushed a commit that referenced this issue May 20, 2019
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants