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: add mustCall() to test-inspector-contexts #30649

Merged
merged 4 commits into from
Nov 28, 2019
Merged

Conversation

Trott
Copy link
Member

@Trott Trott commented Nov 26, 2019

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)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 26, 2019
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@Trott
Copy link
Member Author

Trott commented Nov 26, 2019

Added a second commit to check for errors in session.post() by adding a callback.

@Trott
Copy link
Member Author

Trott commented Nov 26, 2019

Added a third commit to remove destructuring that was making the test harder to read/understand, at least for me.

Added a fourth commit to add logging for a possible/probable infinite loop (or infinite-ish loop) that may be why the test times out sometimes on CI.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@Trott
Copy link
Member Author

Trott commented Nov 26, 2019

@nodejs/testing @nodejs/inspector

@Trott
Copy link
Member Author

Trott commented Nov 27, 2019

@nodejs/collaborators

test/sequential/test-inspector-contexts.js Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 27, 2019
@lundibundi lundibundi removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 27, 2019
@nodejs-github-bot

This comment has been minimized.

@lundibundi lundibundi added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 27, 2019
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member Author

Trott commented Nov 27, 2019

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?

@nodejs-github-bot

This comment has been minimized.

@Trott
Copy link
Member Author

Trott commented Nov 28, 2019

Landed in ea7a6f9...15146e6

@Trott Trott merged commit 15146e6 into nodejs:master Nov 28, 2019
@Trott Trott deleted the contexts branch November 28, 2019 06:40
addaleax pushed a commit that referenced this pull request Nov 30, 2019
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 <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
addaleax pushed a commit that referenced this pull request Nov 30, 2019
If session.post() generates an error, it is currently ignored. Add check
for error by adding a callback to session.post() invocation.

PR-URL: #30649
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
addaleax pushed a commit that referenced this pull request Nov 30, 2019
As I'm working with this test, I'm finding the destructuring of assert
and vm to make this test harder to read/understand. So I'm taking the
liberty of removing them.

PR-URL: #30649
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
addaleax pushed a commit that referenced this pull request Nov 30, 2019
test-inspector-contexts may be entering an infinite loop (or very
long-running loop) in CI, resulting in flakiness. Or maybe not. Add
logging to find out.

PR-URL: #30649
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Dec 1, 2019
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 <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Dec 1, 2019
If session.post() generates an error, it is currently ignored. Add check
for error by adding a callback to session.post() invocation.

PR-URL: #30649
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Dec 1, 2019
As I'm working with this test, I'm finding the destructuring of assert
and vm to make this test harder to read/understand. So I'm taking the
liberty of removing them.

PR-URL: #30649
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Dec 1, 2019
test-inspector-contexts may be entering an infinite loop (or very
long-running loop) in CI, resulting in flakiness. Or maybe not. Add
logging to find out.

PR-URL: #30649
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Dec 3, 2019
@BethGriggs BethGriggs mentioned this pull request Dec 9, 2019
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
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 <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
If session.post() generates an error, it is currently ignored. Add check
for error by adding a callback to session.post() invocation.

PR-URL: #30649
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
As I'm working with this test, I'm finding the destructuring of assert
and vm to make this test harder to read/understand. So I'm taking the
liberty of removing them.

PR-URL: #30649
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
test-inspector-contexts may be entering an infinite loop (or very
long-running loop) in CI, resulting in flakiness. Or maybe not. Add
logging to find out.

PR-URL: #30649
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anto Aravinth <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants