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: fix timeout in sequential/test-fs-watch-system-limit #23692

Closed
wants to merge 3 commits into from

Conversation

mmmscott
Copy link
Contributor

Timeout in this test can occur on Linux if machine is able to handle
200 child processes that are spawned.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 16, 2018
@jasnell
Copy link
Member

jasnell commented Oct 16, 2018

This likely isn't the best way to handle this. Our test runner will generally catch the timeout and fail the test as is.

/cc @nodejs/testing

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fwiw, this is a re-do of #23560

I’m not sure either… it doesn’t seem great (maybe we could skip the test instead of marking it as passing?), but the test is only supposed to check the error message, so this might be okay…

@mmmscott
Copy link
Contributor Author

mmmscott commented Oct 16, 2018

as I was trying alternative options, I attempted to skip the test, but since the error stream expects at least one call, the tests were failing.

I'm happy to make any changes, but off the top of my head I couldn't think of anything that would successful pass the test without upping the number of child processes that could get spawned.

@Trott
Copy link
Member

Trott commented Oct 16, 2018

Would it make sense to move the test file from sequential to pummel and not modify it?

@addaleax
Copy link
Member

@Trott I would be okay with moving it there, yes…

@Trott
Copy link
Member

Trott commented Oct 24, 2018

@marcusscott Would you be OK updating this to move the test unchanged from the sequential directory to the pummel directory instead of adding a timeout? Spawning 200 child processes seems like something that perhaps ought to only happen if the user asks for tests that might strain resources.

@mmmscott
Copy link
Contributor Author

sure thing, makes sense to me!

@Trott
Copy link
Member

Trott commented Nov 6, 2018

jasnell pushed a commit that referenced this pull request Nov 10, 2018
PR-URL: #23692
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Nov 10, 2018

Landed in e492f92

@jasnell jasnell closed this Nov 10, 2018
BridgeAR pushed a commit that referenced this pull request Nov 14, 2018
PR-URL: #23692
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
kiyomizumia pushed a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
PR-URL: nodejs#23692
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this pull request Dec 14, 2018
PR-URL: #23692
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants