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.

Sign up for GitHub

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

Jump to bottom

test: fix unreliable known_issues test #6555

Closed
wants to merge 1 commit into from
Closed

test: fix unreliable known_issues test #6555

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented May 4, 2016

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 Trott added test Issues and PRs related to the tests. known issue test labels May 4, 2016
@joaocgreis
Copy link
Member

LGTM

1 similar comment
@jasnell
Copy link
Member

jasnell commented May 4, 2016

LGTM

@Trott Trott mentioned this pull request May 4, 2016
Closed
2 tasks
santigimeno
santigimeno reviewed May 4, 2016
View reviewed changes
test/known_issues/test-stdout-buffer-flush-on-exit.js
console.log(exponent);
const longLine = lineSeed.repeat(Math.pow(2, exponent));
const cmd = `${process.execPath} ${__filename} child ${exponent}`;
const stdout = execSync(cmd).toString().trim();
Copy link
Member

Choose a reason for hiding this comment

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

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

@santigimeno
Copy link
Member

LGTM with a comment

cjihrig
cjihrig reviewed May 4, 2016
View reviewed changes
test/known_issues/test-stdout-buffer-flush-on-exit.js
const cmd = `${process.execPath} ${__filename} child`;
const stdout = execSync(cmd).toString().trim();
[16, 18, 20].forEach((exponent) => {
console.log(exponent);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@cjihrig
Copy link
Contributor

cjihrig commented May 4, 2016

LGTM with nits addressed.

@addaleax
Copy link
Member

addaleax commented May 4, 2016 •
edited

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

@Trott
Copy link
Member Author

Trott commented May 5, 2016

Nits addressed.

@Trott
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
santigimeno reviewed May 5, 2016
View reviewed changes
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}`;
Copy link
Member

Choose a reason for hiding this comment

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

I think the exponent argument is not needed anymore

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@santigimeno
Copy link
Member

Still LGTM with one nit

Trott added a commit to Trott/io.js that referenced this pull request May 6, 2016
@Trott
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
Copy link
Member Author

Trott commented May 6, 2016

Landed in c4006dd

@Trott Trott closed this May 6, 2016
evanlucas pushed a commit that referenced this pull request May 17, 2016
@Trott @evanlucas
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 evanlucas mentioned this pull request May 17, 2016
Merged
@MylesBorins
Copy link
Member

@Trott lts?

@Trott
Copy link
Member Author

Trott commented Jun 2, 2016

@thealphanerd Yes

MylesBorins pushed a commit that referenced this pull request Jun 14, 2016
@Trott
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 MylesBorins mentioned this pull request Jun 24, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
@Trott
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
@Trott
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 Trott deleted the known branch January 13, 2022 22:43
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
@Trott @joaocgreis @jasnell @santigimeno @cjihrig @addaleax @MylesBorins