Skip to content

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

Closed
lpinca wants to merge 1 commit into
nodejs:masterfrom
lpinca:deflake/test-child-process-pipe-dataflow
Closed

test: deflake child-process-pipe-dataflow#40838
lpinca wants to merge 1 commit into
nodejs:masterfrom
lpinca:deflake/test-child-process-pipe-dataflow

Conversation

@lpinca

@lpincalpinca commented Nov 17, 2021

Copy link
Copy Markdown
Member

Fixes: #25988

targos
targos approved these changes Nov 17, 2021
View reviewed changes

@targostargos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any idea why it suddenly started to fail?

@targostargos added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 17, 2021
@lpinca

lpinca commented Nov 17, 2021

Copy link
Copy Markdown
MemberAuthor

I don't know. Could it be a different version of Git Bash?

@targos

targos commented Nov 17, 2021

Copy link
Copy Markdown
Member

@nodejs/build-infra were the Windows hosts updated recently?

@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 17, 2021
@nodejs-github-bot

nodejs-github-bot commented Nov 17, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/40942/

@targos

targos commented Nov 17, 2021

Copy link
Copy Markdown
Member

There's a linter error.

@lpinca

lpinca commented Nov 17, 2021 via email •
edited
Loading

Copy link
Copy Markdown
MemberAuthor

mhdawson
mhdawson reviewed Nov 17, 2021
View reviewed changes
Comment threadtest/parallel/test-child-process-pipe-dataflow.js Outdated

@mhdawsonmhdawsonNov 17, 2021

Copy link
Copy Markdown
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.

@lpincalpincaNov 17, 2021•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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.

@mhdawsonmhdawsonNov 17, 2021

Copy link
Copy Markdown
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'.

@lpincalpincaNov 17, 2021•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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.

@mhdawsonmhdawsonNov 17, 2021

Copy link
Copy Markdown
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.

@mhdawsonmhdawsonNov 17, 2021

Copy link
Copy Markdown
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.

@lpinca
lpincaforce-pushed the deflake/test-child-process-pipe-dataflow branch from cfe7395 to 5739f62CompareNovember 17, 2021 14:59
@tniessen

tniessen commented Nov 17, 2021

Copy link
Copy Markdown
Member

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.

mhdawson
mhdawson reviewed Nov 17, 2021
View reviewed changes
Comment threadtest/parallel/test-child-process-pipe-dataflow.js Outdated
mhdawson
mhdawson approved these changes Nov 17, 2021
View reviewed changes

@mhdawsonmhdawson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM with a suggestion

@lpinca
lpincaforce-pushed the deflake/test-child-process-pipe-dataflow branch from 5739f62 to b175af8CompareNovember 17, 2021 16:06
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@targostargos added fast-track PRs that do not need to wait for 48 hours to land. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 17, 2021
@targos

targos commented Nov 17, 2021

Copy link
Copy Markdown
Member

Requesting fast-tracking because it fixes CI.

@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 17, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Nov 17, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Nov 17, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/40948/

@nodejs-github-bot

nodejs-github-bot commented Nov 17, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/40952/

@TrottTrott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 17, 2021
@github-actions

github-actionsBot commented Nov 17, 2021

Copy link
Copy Markdown
Contributor

Fast-track has been requested by @targos. Please 👍 to approve.

@github-actionsgithub-actionsBot mentioned this pull request Nov 18, 2021
Open
36 tasks
@Trott

Trott commented Nov 18, 2021

Copy link
Copy Markdown
Member

Landed in 0c2011c

@TrottTrott closed this Nov 18, 2021
Trott pushed a commit that referenced this pull request Nov 18, 2021
@lpinca@Trott
test: deflake child-process-pipe-dataflow
0c2011c
Fixes: #25988
PR-URL: #40838
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@lpinca
lpinca deleted the deflake/test-child-process-pipe-dataflow branch November 18, 2021 06:12
This was referenced Nov 19, 2021
Open
Open
Open
targos pushed a commit that referenced this pull request Nov 21, 2021
@lpinca@targos
test: deflake child-process-pipe-dataflow
896073b
Fixes: #25988
PR-URL: #40838
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
This was referenced Nov 22, 2021
Open
Open
richardlau pushed a commit that referenced this pull request Nov 23, 2021
@lpinca@richardlau
test: deflake child-process-pipe-dataflow
7479447
Fixes: #25988
PR-URL: #40838
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
richardlau pushed a commit that referenced this pull request Nov 23, 2021
@lpinca@richardlau
test: deflake child-process-pipe-dataflow
0682370
Fixes: #25988
PR-URL: #40838
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
richardlau pushed a commit that referenced this pull request Nov 25, 2021
@lpinca@richardlau
test: deflake child-process-pipe-dataflow
704989b
Fixes: #25988
PR-URL: #40838
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@richardlaurichardlau mentioned this pull request Nov 25, 2021
Merged
@targostargos mentioned this pull request Nov 26, 2021
Merged
BethGriggs pushed a commit that referenced this pull request Nov 29, 2021
@lpinca@BethGriggs
test: deflake child-process-pipe-dataflow
01ffe03
Fixes: #25988
PR-URL: #40838
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BethGriggsBethGriggs mentioned this pull request Nov 30, 2021
Merged
1 task
@richardlaurichardlau mentioned this pull request Dec 13, 2021
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott approved these changes
@targostargostargos approved these changes
@tniessentniessentniessen approved these changes
+1 more reviewer
@mhdawsonmhdawsonmhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.fast-trackPRs that do not need to wait for 48 hours to land.needs-ciPRs that need a full CI run.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

flaky: test-child-process-pipe-dataflow

6 participants

@lpinca@targos@nodejs-github-bot@tniessen@Trott@mhdawson