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: minimize time for child_process benchmark #12518

Closed
wants to merge 3 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Apr 19, 2017

test-benchmark-child-process sometimes times out on Windows in CI.
Minimize the number of configurations that run so it will (hopefully)
not time out anymore.

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

test child_process benchmark

test-benchmark-child-process sometimes times out on Windows in CI.
Minimize the number of configurations that run so it will (hopefully)
not time out anymore.
@Trott Trott added benchmark Issues and PRs related to the benchmark subsystem. child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests. labels Apr 19, 2017
@Trott
Copy link
Member Author

Trott commented Apr 19, 2017

cc @joyeecheung

@Trott
Copy link
Member Author

Trott commented Apr 19, 2017

I'd like to get this landed sooner than 48 hours because Windows CI is failing a lot because of this right now. @nodejs/testing @nodejs/build

@Trott
Copy link
Member Author

Trott commented Apr 19, 2017

@Trott
Copy link
Member Author

Trott commented Apr 19, 2017

Hmmm....CI still timed out on one node, but also had another odd timeout failure.

Took only 2 seconds to run on a machine where it passed... wonder if we just need to clean up some hosts first...maybe something is just odd with test-azure_msft-win2016-x64-4 or something like that...

Running CI again: https://ci.nodejs.org/job/node-test-pull-request/7527/

@Trott
Copy link
Member Author

Trott commented Apr 19, 2017

Trying with execSync rather than exec to see if that helps at all.

CI: https://ci.nodejs.org/job/node-test-pull-request/7528/

@Trott
Copy link
Member Author

Trott commented Apr 19, 2017

If the above CI jobs don't run cleanly, the thing to do might be to move the test to disabled until we figure out how to make it reliable on Windows. I would have to suspect a bug in the actual benchmark code and not in the test but ¯\(ツ)/¯.

Anyway, hopefully one or both of the above CI runs comes out green and we can at least try this.

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 19, 2017

@Trott Are we sure we have yes.exe in this machine in PATH? Or can we test its functionality aside?

@Trott
Copy link
Member Author

Trott commented Apr 19, 2017

@Trott Are we sure we have yes.exe in this machine in PATH?

No idea. Guessing not. @nodejs/build?

Guess that may be the problem. Now wondering if this landed without a clean CI run in the first place...

@Trott
Copy link
Member Author

Trott commented Apr 19, 2017

@vsemozhetbyt Took your comment and ran with it: #12326 (comment)

@Trott
Copy link
Member Author

Trott commented Apr 19, 2017

Although I don't think it would hang if that's the case...it would exit with ENOENT, I think.

@Trott
Copy link
Member Author

Trott commented Apr 19, 2017

No real reason to think this will make a huge difference, but set dur=0 because we can. In the likely event this doesn't solve the problem, I'll move the test to disabled or revert the commit that added it.

CI: https://ci.nodejs.org/job/node-test-pull-request/7529/

@Trott
Copy link
Member Author

Trott commented Apr 19, 2017

sigh Windows build failure. Let's try again.

CI: https://ci.nodejs.org/job/node-test-pull-request/7530/

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 19, 2017

What if we --filter these 5 benchmarks one by one here to detect the source of the problem?

@Trott
Copy link
Member Author

Trott commented Apr 19, 2017

Set dur=0 seems to have fixed it, although a stress test is probably in order.

Also, dur=0.1 really shouldn't break, so doing what @vsemozhetbyt and trying to figure out which benchmark is blowing up is good too. We can do it in CI, but if someone on @nodejs/platform-windows or @nodejs/build happens to have an accessible Windows 2016 machine and figure out which ones are hanging, that would be more awesome and efficient.

@Trott
Copy link
Member Author

Trott commented Apr 19, 2017

Stress test for this PR: https://ci.nodejs.org/job/node-stress-single-test/1156/nodes=win2016/console

Stress test for master (should show failures): https://ci.nodejs.org/job/node-stress-single-test/1157/nodes=win2016/console

Hopefully I didn't make any typos when filling out the Jenkins form...

@Trott
Copy link
Member Author

Trott commented Apr 19, 2017

Also, when it's done waiting for one of the other stress jobs to finish, this should figure out which of the five benchmarks are the issue: https://ci.nodejs.org/job/node-stress-single-test/1159/nodes=win2016/ whoops I had an error in the branch, will try again...

@Trott
Copy link
Member Author

Trott commented Apr 19, 2017

Stress tests are behaving as expected. This branch has run more than 40 times without a problem. Will let it go a bit longer. Master has timed out twice without a successful run so far.

@Trott
Copy link
Member Author

Trott commented Apr 19, 2017

Stress test to identify which of the benchmarks is causing problems: https://ci.nodejs.org/job/node-stress-single-test/1161/nodes=win2016/console

@Trott
Copy link
Member Author

Trott commented Apr 19, 2017

Meantime: Landing to fix CI.

Trott added a commit to Trott/io.js that referenced this pull request Apr 19, 2017
test-benchmark-child-process sometimes times out on Windows in CI.
Minimize the number of configurations that run so it will (hopefully)
not time out anymore. Set dur=0.

PR-URL: nodejs#12518
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@Trott
Copy link
Member Author

Trott commented Apr 19, 2017

Landed in 32f77ff. CI should be back to green now.

@Trott Trott closed this Apr 19, 2017
@Trott
Copy link
Member Author

Trott commented Apr 20, 2017

Interestingly, looking at the results on Windows right now for the split out tests, it seems like it may have been a genuine timeout and not anything hanging. Splitting the test into five pieces, they all pass, but take about 30 seconds each to run. Yikes! That seems...odd... https://ci.nodejs.org/job/node-stress-single-test/1161/nodes=win2016/console

Gotta run right now, but maybe if I get some time later, I'll spin up a similar job on another Windows machine and maybe a Linux machine too and compare the run times.

@joyeecheung
Copy link
Member

joyeecheung commented Apr 21, 2017

Sorry for being a bit late. I think we can probably make two tests for each set of benchmarks, one setting numeric configurations to 0 (mostly for testing if there are any syntax errors) and one setting those to 1 or a value as small as possible (see if they actually run properly). If we ever have to disable the second one, at least we will have the first one in place...

evanlucas pushed a commit that referenced this pull request Apr 25, 2017
test-benchmark-child-process sometimes times out on Windows in CI.
Minimize the number of configurations that run so it will (hopefully)
not time out anymore. Set dur=0.

PR-URL: #12518
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@evanlucas evanlucas mentioned this pull request May 1, 2017
evanlucas pushed a commit that referenced this pull request May 1, 2017
test-benchmark-child-process sometimes times out on Windows in CI.
Minimize the number of configurations that run so it will (hopefully)
not time out anymore. Set dur=0.

PR-URL: #12518
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
test-benchmark-child-process sometimes times out on Windows in CI.
Minimize the number of configurations that run so it will (hopefully)
not time out anymore. Set dur=0.

PR-URL: #12518
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
@gibfahn gibfahn added baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v6.x labels May 16, 2017
@gibfahn
Copy link
Member

gibfahn commented May 16, 2017

Should land with #12326.

@MylesBorins MylesBorins removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Aug 17, 2018
@Trott Trott deleted the cp-bench-test branch January 13, 2022 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. child_process Issues and PRs related to the child_process subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants