test: fix test-sync-io-option #1840
Conversation
|
cc @trevnorris |
test/parallel/test-sync-io-option.js
Outdated
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. |
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.
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 <[email protected]> Reviewed-By: Trevor Norris <[email protected]>
evanlucas
force-pushed
the
fix-sync-io-test
branch
from
e199dd9 to
43a82f8
Compare
|
Landed in 43a82f8. Thanks! |
5 participants
This test was failing occasionally both locally and on CI. Switched
from using spawn to execFile for a more reliable test.
Related: #1837