FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: fix unreliable known_issues test by Trott · Pull Request #6555 · nodejs/node · GitHub

/ node Public

test: fix unreliable known_issues test - #6555

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:known
ConversationCommits (1)ChecksFiles changed
Closed

test: fix unreliable known_issues test#6555
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:known

Conversation

Trott commented May 4, 2016

Copy link
Copy Markdown
Member
Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

test-stdout-buffer-flush-on-exit was not failing reliably on POSIX
machines and not failing at all on Windows. Revised test fails reliably
on POSIX and is skipped (in CI) on Windows where the issue does not
exist.

Fixes: #6527

Trott added test Issues and PRs related to the tests. known issue test labels May 4, 2016

joaocgreis commented May 4, 2016

Copy link
Copy Markdown
Member

LGTM

1 similar comment

jasnell commented May 4, 2016

Copy link
Copy Markdown
Member

LGTM

Trott mentioned this pull request May 4, 2016
Closed
2 tasks
santigimeno reviewed May 4, 2016
View reviewed changes
Comment thread test/known_issues/test-stdout-buffer-flush-on-exit.js Outdated

santigimeno May 4, 2016 •
edited
Loading

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can't you just calculate Math.pow(2, exponent) once in the parent and pass it as argument to the child?

santigimeno commented May 4, 2016

Copy link
Copy Markdown
Member

LGTM with a comment

cjihrig reviewed May 4, 2016
View reviewed changes
Comment thread test/known_issues/test-stdout-buffer-flush-on-exit.js Outdated

cjihrig May 4, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is the console.log() needed, or can it be dropped?

addaleax May 4, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don’t know if it’s needed, but I’d like to have some way of knowing at what length the test failed after it has run.

Trott May 4, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I accidentally left it in from debugging. I can add it to the assert message, perhaps.

cjihrig commented May 4, 2016

Copy link
Copy Markdown
Contributor

LGTM with nits addressed.

addaleax commented May 4, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

LGTM, maybe with @santigimeno’s nit addressed if that works too.

Trott commented May 5, 2016

Copy link
Copy Markdown
Member Author

Nits addressed.

test: fix unreliable known_issues test
e594ea5 
`test-stdout-buffer-flush-on-exit` was not failing reliably on POSIX
machines and not failing at all on Windows. Revised test fails reliably
on POSIX and is skipped (in CI) on Windows where the issue does not
exist.

Fixes: nodejs#6527
santigimeno reviewed May 5, 2016
View reviewed changes
Comment thread test/known_issues/test-stdout-buffer-flush-on-exit.js
[16, 18, 20].forEach((exponent) => {
const bigNum = Math.pow(2, exponent);
const longLine = lineSeed.repeat(bigNum);
const cmd = `${process.execPath} ${__filename} child ${exponent} ${bigNum}`;

santigimeno May 5, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think the exponent argument is not needed anymore

Trott May 6, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Strictly speaking, that's true, but I left it in for the assert message.

santigimeno commented May 5, 2016

Copy link
Copy Markdown
Member

Still LGTM with one nit

Trott added a commit to Trott/io.js that referenced this pull request May 6, 2016
test: fix unreliable known_issues test
c4006dd 
`test-stdout-buffer-flush-on-exit` was not failing reliably on POSIX
machines and not failing at all on Windows. Revised test fails reliably
on POSIX and is skipped (in CI) on Windows where the issue does not
exist.

Fixes: nodejs#6527
PR-URL: nodejs#6555
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joao Reis <reis@janeasystems.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>

Trott commented May 6, 2016

Copy link
Copy Markdown
Member Author

Landed in c4006dd

Trott closed this May 6, 2016
evanlucas pushed a commit that referenced this pull request May 17, 2016
test: fix unreliable known_issues test
83dab80 
`test-stdout-buffer-flush-on-exit` was not failing reliably on POSIX
machines and not failing at all on Windows. Revised test fails reliably
on POSIX and is skipped (in CI) on Windows where the issue does not
exist.

Fixes: #6527
PR-URL: #6555
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joao Reis <reis@janeasystems.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
evanlucas mentioned this pull request May 17, 2016
Merged

MylesBorins commented Jun 2, 2016

Copy link
Copy Markdown
Contributor

@Trott lts?

Trott commented Jun 2, 2016

Copy link
Copy Markdown
Member Author

@thealphanerd Yes

MylesBorins pushed a commit that referenced this pull request Jun 14, 2016
test: fix unreliable known_issues test
c6d8b7c 
`test-stdout-buffer-flush-on-exit` was not failing reliably on POSIX
machines and not failing at all on Windows. Revised test fails reliably
on POSIX and is skipped (in CI) on Windows where the issue does not
exist.

Fixes: #6527
PR-URL: #6555
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joao Reis <reis@janeasystems.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
MylesBorins mentioned this pull request Jun 24, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
test: fix unreliable known_issues test
be0bb5f 
`test-stdout-buffer-flush-on-exit` was not failing reliably on POSIX
machines and not failing at all on Windows. Revised test fails reliably
on POSIX and is skipped (in CI) on Windows where the issue does not
exist.

Fixes: #6527
PR-URL: #6555
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joao Reis <reis@janeasystems.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
test: fix unreliable known_issues test
91e7b01 
`test-stdout-buffer-flush-on-exit` was not failing reliably on POSIX
machines and not failing at all on Windows. Revised test fails reliably
on POSIX and is skipped (in CI) on Windows where the issue does not
exist.

Fixes: #6527
PR-URL: #6555
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joao Reis <reis@janeasystems.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Trott deleted the known branch January 13, 2022 22:43
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

failing known-issues tests

7 participants

Back | FazBrowse Home | New Git URL