test: deflake child-process-pipe-dataflow#40838
Uh oh!
There was an error while loading. Please reload this page.
Conversation
targos left a comment
targos left a comment There was a problem hiding this comment.
Any idea why it suddenly started to fail?
lpinca commented Nov 17, 2021
I don't know. Could it be a different version of Git Bash? |
targos commented Nov 17, 2021
@nodejs/build-infra were the Windows hosts updated recently? |
github-actionsbot removed the request-ci 
nodejs-github-bot commented Nov 17, 2021
targos commented Nov 17, 2021
There's a linter error. |
Yes, it’s probably the no longer used `os` variable. Will fix it in a bit. … On 17 Nov 2021, at 14:01, Michaël Zasso ***@***.***> wrote: There's a linter error. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. |
| // So cut the buffer into lines at some points, forcing | ||
| // data flow to be split in the stream. | ||
| for(leti=1;i<KB;i++) | ||
| buf.write(os.EOL,i*KB); |
There was a problem hiding this comment.
What is the value of os.EOL? For windows I expecte it to be \r\n and wonder why cutting it down to just \n allows the test to pass.
There was a problem hiding this comment.
If you look at the error message here: https://ci.nodejs.org/job/node-test-binary-windows-js-suites/12290/RUN_SUBSET=3,nodes=win2012r2-COMPILED_BY-vs2019-x86/testReport/junit/(root)/test/parallel_test_child_process_pipe_dataflow/, you'll notice that the difference between the actual and the expected character count is 1023. That error can only happen if there is an additional (or missing) character per line, so I guess for some reason \r is not handled correctly.
There was a problem hiding this comment.
That would make sense if somewhere in the flow '\r\n' is getting converted to '\n'.
There was a problem hiding this comment.
I think it's the grep command not playing well with \r\n.
There was a problem hiding this comment.
I don't see any issues related to updating the Windows machines, but I'm not sure if they would update themselves @joaocgreis do you know.
Either way its worth getting the tests running again as long as this passes on all windows machines.
There was a problem hiding this comment.
I don't see any recent PRs that might have changed related behaviour in Node.js itself.
lpincaforce-pushed the deflake/test-child-process-pipe-dataflow branch from cfe7395 to 5739f62Compare
tniessen commented Nov 17, 2021
I remember having problems with the non-Windows tools under Windows a while ago. Eventually, we might want to switch to binaries that are native to Windows and not provided by msys. |
Uh oh!
There was an error while loading. Please reload this page.
lpincaforce-pushed the deflake/test-child-process-pipe-dataflow branch from 5739f62 to b175af8Compare This comment has been minimized.
This comment has been minimized.
targos added fast-track targos commented Nov 17, 2021
Requesting fast-tracking because it fixes CI. |
github-actionsbot removed the request-ci This comment has been minimized.
This comment has been minimized.
nodejs-github-bot added needs-ci This comment has been minimized.
This comment has been minimized.
nodejs-github-bot commented Nov 17, 2021
nodejs-github-bot commented Nov 17, 2021
Trott added the author ready 
Fast-track has been requested by @targos. Please 👍 to approve. |
Trott commented Nov 18, 2021
Landed in 0c2011c |
Fixes: #25988 PR-URL: #40838 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Fixes: #25988 PR-URL: #40838 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Fixes: #25988 PR-URL: #40838 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Fixes: #25988 PR-URL: #40838 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Fixes: #25988 PR-URL: #40838 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Fixes: #25988 PR-URL: #40838 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]>
6 participants
Fixes: #25988