-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Conversation
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.
cc @joyeecheung |
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 |
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/ |
Trying with |
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. |
@Trott Are we sure we have |
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... |
@vsemozhetbyt Took your comment and ran with it: #12326 (comment) |
Although I don't think it would hang if that's the case...it would exit with ENOENT, I think. |
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 |
sigh Windows build failure. Let's try again. |
What if we |
Set Also, |
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... |
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: |
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. |
Stress test to identify which of the benchmarks is causing problems: https://ci.nodejs.org/job/node-stress-single-test/1161/nodes=win2016/console |
Meantime: Landing to fix CI. |
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>
Landed in 32f77ff. CI should be back to green now. |
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. |
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... |
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>
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>
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>
Should land with #12326. |
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), orvcbuild test
(Windows) passesAffected core subsystem(s)
test child_process benchmark