Skip to content

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented Jun 4, 2023

Fixes: #48300

seems like something changed in the Windows machines causing wc to behave differently
changed from wc -c (count bytes) to wc -m (count characters).

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jun 4, 2023
@MoLow MoLow force-pushed the fix-child-process-data-flow branch from dfe256d to 583f7e6 Compare June 4, 2023 07:39
@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 4, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 4, 2023
@nodejs-github-bot
Copy link
Collaborator

@lpinca
Copy link
Member

lpinca commented Jun 4, 2023

By the way the test passes with wc -c on GitHub Actions, so I think it is an issue with our Windows machines or the tools used there.

@MoLow
Copy link
Member Author

MoLow commented Jun 4, 2023

By the way the test passes with wc -c on GitHub Actions, so I think it is an issue with our Windows machines or the tools used there.

Yep. passed by me locally on a windows vm as well

@aduh95
Copy link
Contributor

aduh95 commented Jun 4, 2023

And it used to pass on Jenkins until a few days ago

@MoLow
Copy link
Member Author

MoLow commented Jun 4, 2023

this fix only fixes the issue for some windows versions: https://ci.nodejs.org/job/node-test-binary-windows-js-suites/21245/

@bricss
Copy link
Contributor

bricss commented Jun 4, 2023

As an option for quickfix 🩹 it would be the use of wc -cm and match with one of the output columns 🙂

@MoLow
Copy link
Member Author

MoLow commented Jun 4, 2023

I think we can implement wc ourselves, it will be less hacky than passing unexpected flags:
`node -p "process.stdin.toArray().then(arr => console.log(arr.join("").length))"

@lpinca
Copy link
Member

lpinca commented Jun 4, 2023

I think that invalidates the original intention of the test as we could do the same for cat and grep.

@MoLow
Copy link
Member Author

MoLow commented Jun 4, 2023

Well I guess the best solution is to figure out why wc started returning these numbers on windows :|

@lpinca
Copy link
Member

lpinca commented Jun 4, 2023

Or mark it flaky until we do :).

@MoLow MoLow closed this Jun 4, 2023
@MoLow MoLow deleted the fix-child-process-data-flow branch June 4, 2023 18:11
@bricss
Copy link
Contributor

bricss commented Jun 4, 2023

IMO, wc -cm with assert.match(wcBuf.trim(), new RegExp(String.raw`\b${ MB + 1 }\b`)) might do the trick 🤹

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

test-child-process-pipe-dataflow failing on Windows
5 participants