Skip to content

Conversation

@MoLow
Copy link
Member

@MoLowMoLow 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-botnodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jun 4, 2023
@MoLowMoLowforce-pushed the fix-child-process-data-flow branch from dfe256d to 583f7e6CompareJune 4, 2023 07:39
@MoLowMoLow added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 4, 2023
@github-actionsgithub-actionsbot 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
MemberAuthor

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

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

@MoLow
Copy link
MemberAuthor

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

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

@MoLow
Copy link
MemberAuthor

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

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

@MoLow
Copy link
MemberAuthor

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

Or mark it flaky until we do :).

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

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

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ciPRs that need a full CI run.testIssues 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

@MoLow@nodejs-github-bot@lpinca@aduh95@bricss