test: fix test-sync-io-option #1840
Conversation
|
cc @trevnorris |
| if (flags.length > 0) | ||
| setImmediate(runTest, flags); | ||
| }); | ||
| }(['--trace-sync-io', ' '])); | ||
| }(['--trace-sync-io', '--trace-deprecation'])); |
There was a problem hiding this comment.
Shouldn't we still test without flags?
There was a problem hiding this comment.
The test was throwing an error in the first place. I can add a test without flags though
evanlucas
force-pushed
the
fix-sync-io-test
branch
from
2bb0fb7
to
e199dd9
Compare
|
Ok, the test now runs with the flag and without. |
|
Fine by me. |
| if (flags.length > 0) | ||
| setImmediate(runTest, flags); | ||
| }); | ||
| }(['--trace-sync-io', ' '])); | ||
| }(['--trace-sync-io', ''])); |
There was a problem hiding this comment.
Why is this flags game and IIFE required?
There was a problem hiding this comment.
When I wrote it I wanted each run to have it's own running context. So I kept most variables local to the IIFE so there wouldn't be any accidental interference. Just a technique to help write more reliable tests.
| if (/^WARNING[\s\S]*fs\.readFileSync/.test(stderr)) | ||
| cntr++; | ||
| if (args[0] === '--trace-sync-io') { | ||
| assert.equal(cntr, 1); |
There was a problem hiding this comment.
Why did this change from 4 to 1 warning?
There was a problem hiding this comment.
It changed because we only get stderr in the callback one time for each call. Previously it was in a data event which was emit more than once.
|
LGTM, should CI this. |
|
@trevnorris care to sign this off too? |
|
There is one failure, but seems unrelated |
|
LGTM |
This test was failing occasionally both locally and on CI. Switched from using spawn to execFile for a more reliable test. Fixes: nodejs#1837 PR-URL: nodejs#1840 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Trevor Norris <trev.norris@gmail.com>
evanlucas
force-pushed
the
fix-sync-io-test
branch
from
e199dd9
to
43a82f8
Compare
|
Landed in 43a82f8. Thanks! |
This test was failing occasionally both locally and on CI. Switched
from using spawn to execFile for a more reliable test.
Related: #1837