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.

Sign up for GitHub

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

Jump to bottom

investigate flaky test-inspector-contexts in CI #30519

Closed
gireeshpunathil opened this issue Nov 18, 2019 · 8 comments
Closed

investigate flaky test-inspector-contexts in CI #30519

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

Comments

@gireeshpunathil
Copy link
Member

  • Version:
  • Platform:
  • Subsystem:

sequential/test-inspector-contexts is flaking (timing out) a lot in the CI. Discovered while I was running CI for progressing the C&L PRs.

Example failure:

13:05:40 not ok 1307 sequential/test-inspector-contexts
13:05:40   ---
13:05:40   duration_ms: 148.541
13:05:40   severity: fail
13:05:40   exitcode: 1
13:05:40   stack: |-
13:05:40     timeout
13:05:40     Testing context created/destroyed notifications
13:05:40   ...

https://ci.nodejs.org/job/node-test-binary-windows-2/COMPILED_BY=vs2017,RUNNER=win2016,RUN_SUBSET=0/4208/console

cc @nodejs/testing @nodejs/v8-inspector
@Trott
Copy link
Member

Trott commented Nov 26, 2019

Happened again today.

https://ci.nodejs.org/job/node-test-binary-windows-2/4494/COMPILED_BY=vs2017,RUNNER=win2016,RUN_SUBSET=1/console

21:44:04 not ok 1311 sequential/test-inspector-contexts
21:44:04   ---
21:44:04   duration_ms: 150.925
21:44:04   severity: fail
21:44:04   exitcode: 1
21:44:04   stack: |-
21:44:04     timeout
21:44:04     Testing context created/destroyed notifications
21:44:04   ...

test-azure_msft-win2016-x64-3

@Trott
Copy link
Member

Trott commented Nov 26, 2019

I wonder if session.post() is failing but since no callback is sent, the error is never received?

@Trott
Copy link
Member

Trott commented Nov 26, 2019

I notice that if I modify mainContextPromise such that it never executes, the test still passes. Will add a common.mustCall() to make sure the test suites actually run.

@Trott Trott mentioned this issue Nov 26, 2019
Merged
2 tasks
@Trott
Copy link
Member

Trott commented Nov 26, 2019 •
edited
Loading

Confirmed that if session.post() errors, it is ignored with no callback. Will add that to the PR.

This was referenced Nov 26, 2019
Closed
Closed
Closed
@Trott
Copy link
Member

Trott commented Nov 27, 2019

#30649 (comment)

Welp, this at least has the desired effect of supplying more information about why test-inspector-contexts sometimes hangs in CI.

13:29:09 not ok 1312 sequential/test-inspector-contexts
13:29:09   ---
13:29:09   duration_ms: 149.140
13:29:09   severity: fail
13:29:09   exitcode: 1
13:29:09   stack: |-
13:29:09     timeout
13:29:09     Testing context created/destroyed notifications
13:29:09     Checking/waiting for GC.
13:29:09   ...

So it looks like it just runs the first global.gc() over and over and the context is never garbage collected/destroyed?

@Trott
Copy link
Member

Trott commented Nov 27, 2019

@nodejs/inspector @nodejs/vm @nodejs/testing Any thoughts on what to do next to troubleshoot?

Trott added a commit to Trott/io.js that referenced this issue Nov 28, 2019
@Trott
test: add mustCall() to test-inspector-contexts
80c170e 
In test-inspector-contexts, if mainContextPromise is modified such that
the `method` argument is changed to something invalid, the test still
passes and the second part of the test never runs. Use
`common.mustCall()` to cause the test to fail if the second part of the
test does not run.

Refs: nodejs#30519 (comment)

PR-URL: nodejs#30649
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
addaleax pushed a commit that referenced this issue Nov 30, 2019
@Trott @addaleax
test: add mustCall() to test-inspector-contexts
3769632 
In test-inspector-contexts, if mainContextPromise is modified such that
the `method` argument is changed to something invalid, the test still
passes and the second part of the test never runs. Use
`common.mustCall()` to cause the test to fail if the second part of the
test does not run.

Refs: #30519 (comment)

PR-URL: #30649
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this issue Dec 1, 2019
@Trott @targos
test: add mustCall() to test-inspector-contexts
23ad927 
In test-inspector-contexts, if mainContextPromise is modified such that
the `method` argument is changed to something invalid, the test still
passes and the second part of the test never runs. Use
`common.mustCall()` to cause the test to fail if the second part of the
test does not run.

Refs: #30519 (comment)

PR-URL: #30649
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this issue Dec 17, 2019
@Trott @MylesBorins
test: add mustCall() to test-inspector-contexts
c3ad977 
In test-inspector-contexts, if mainContextPromise is modified such that
the `method` argument is changed to something invalid, the test still
passes and the second part of the test never runs. Use
`common.mustCall()` to cause the test to fail if the second part of the
test does not run.

Refs: #30519 (comment)

PR-URL: #30649
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@targos targos added the flaky-test Issues and PRs related to the tests with unstable failures on the CI. label Dec 26, 2020
@targos
Copy link
Member

targos commented Dec 26, 2020

Is this still relevant?

@Trott
Copy link
Member

Trott commented Jan 6, 2021

Is this still relevant?

I'm not seeing it using ncu-ci walk pr or ncu-ci walk commit so I'm going to optimistically close this.

@Trott Trott closed this as completed Jan 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@Trott @targos @gireeshpunathil