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.

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

test: fix test-child-process-pipe-dataflow #48331

Closed
wants to merge 1 commit into from

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

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

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