test: Fixes test-tick-processor to not hang on Windows #7628
Conversation
| @@ -57,4 +60,5 @@ function runTest(pattern, code) { | |||
| {encoding: 'utf8'}); | |||
| assert(pattern.test(out)); | |||
| fs.unlinkSync(log); | |||
| } | |||
| fs.unlinkSync(testCodePath); | |||
| } | |||
There was a problem hiding this comment.
Missing line feed at the end of the file should result in a lint error if you run make jslint or make test.
There was a problem hiding this comment.
Or vcbuild jslint for us Windows folks :)
| @@ -18,7 +19,7 @@ common.refreshTmpDir(); | |||
| process.chdir(common.tmpDir); | |||
| // Unknown checked for to prevent flakiness, if pattern is not found, | |||
| // then a large number of unknown ticks should be present | |||
| runTest(/LazyCompile.*\[eval\]:1|.*% UNKNOWN/, | |||
| runTest(/LazyCompile.*test.js:1|.*% UNKNOWN/, | |||
There was a problem hiding this comment.
This test is designed to test the names of profiled functions appear in profiler output (as an approximation to profiler correctness). eval was used before since eval should appear on the stack every time it is observed by the profiler making it most likely to appear in the output. If the script below is now longer eval'd, we'll have to look for another symbol. f is too generic so maybe the function below could be renamed to something unique that the output could be checked for.
There was a problem hiding this comment.
Urgh, so maybe the reason this makes it reliable is because the name of the file is always going to be there. I guess we'll see when the change is made.
Regardless, might be a good idea to preserve all this information-sharing in a comment or seven in the file.
There was a problem hiding this comment.
In testing this, the file name appears in the same place that [eval] did in the original test's output, which I why I replaced it that way. It's not a big deal to make the function name more meaningful and then to adjust the regex if we think that's more declarative.
| @@ -59,6 +64,6 @@ function runTest(pattern, code) { | |||
| ' --prof-process --call-graph-size=10 ' + log, | |||
| {encoding: 'utf8'}); | |||
| assert(pattern.test(out)); | |||
| fs.unlinkSync(log); | |||
| //fs.unlinkSync(log); | |||
| fs.unlinkSync(testCodePath); | |||
There was a problem hiding this comment.
I might have mis-commented before but this is backwards. The log file needs to be removed. The file in the tmp dir (testCodePath) does not. (The tmp directory is cleared by each test that needs it before it is used.)
There was a problem hiding this comment.
I think I misread it. Fixed.
|
This looks good on my end if the flakiness is still gone with the latest changes (checking for function name instead of file name). I would expect the file name to appear somewhere in the output unconditionally so it would be worth reverifying. |
| @@ -60,7 +60,7 @@ rules: | |||
| indent: [2, 2, {SwitchCase: 1}] | |||
| key-spacing: [2, {mode: "minimum"}] | |||
| keyword-spacing: 2 | |||
| linebreak-style: [2, "unix"] | |||
| # linebreak-style: [2, "unix"] | |||
There was a problem hiding this comment.
This change should be removed from the change set, no?
|
Failed in CI: https://ci.nodejs.org/job/node-test-binary-arm/2796/RUN_SUBSET=6,label=pi1-raspbian-wheezy/console I don't think this change is going to make the test not flaky, unfortunately. Although if your understanding of the situation has increased and you have other ideas to try, please do! |
|
Closing as test-tick-processor has been broken into three different test files now and remaining flakiness is being explored in #8725. (Re-open or comment if you think this shouldn't be closed.) |
Checklist
make -j4 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
This adjusts the parallel/test-tick-processor test so that it doesn't hang on Windows when run during stress testing (#4427)
Description of change
The change was to not use fs.execFileSync and instead just use fs.execSync after wiring the test code to a file. I ran the adjusted test on Windows 10 x64 and Windows Server 2012 R2 doing 3000 iterations on each OS.
** I have no idea why this fixes the hanging issue **
cc: @Trott