Skip to content
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.

Sign up for GitHub

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

Jump to bottom

test: deflake child-process-pipe-dataflow #40838

Closed
wants to merge 1 commit into from
Closed

test: deflake child-process-pipe-dataflow #40838

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 6 additions & 6 deletions test/parallel/test-child-process-pipe-dataflow.js
View file
Open in desktop
Expand Up @@ -3,7 +3,6 @@ const common = require('../common');
const assert = require('assert');
const path = require('path');
const fs = require('fs');
const os = require('os');
const spawn = require('child_process').spawn;
const tmpdir = require('../common/tmpdir');

Expand All @@ -22,12 +21,13 @@ const MB = KB * KB;
const file = path.resolve(tmpdir.path, 'data.txt');
const buf = Buffer.alloc(MB).fill('x');

// Most OS commands that deal with data, attach special
// meanings to new line - for example, line buffering.
// So cut the buffer into lines at some points, forcing
// data flow to be split in the stream.
// Most OS commands that deal with data, attach special meanings to new line -
// for example, line buffering. So cut the buffer into lines at some points,
// forcing data flow to be split in the stream. Do not use os.EOL for \n as
// that is 2 characters on Windows and is sometimes converted to 1 character
// which causes the test to fail.
for (let i = 1; i < KB; i++)
buf.write(os.EOL, i * KB);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@lpinca lpinca Nov 17, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would make sense if somewhere in the flow '\r\n' is getting converted to '\n'.

Copy link
Member Author

@lpinca lpinca Nov 17, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's the grep command not playing well with \r\n.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any recent PRs that might have changed related behaviour in Node.js itself.

buf.write('\n', i * KB);
lpinca marked this conversation as resolved.
Show resolved Hide resolved
fs.writeFileSync(file, buf.toString());

cat = spawn('cat', [file]);
Expand Down