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: pummel test fixes #4998

Closed
wants to merge 2 commits into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 31, 2016

This fixes some variable redeclarations in pummel tests and, in the
process, gets test-stream-pipe-multi.js to run again. It had been
failing to run.

This fixes some variable redeclarations in pummel tests and, in the
process, gets `test-stream-pipe-multi.js` to run again. It had been
failing to run.
@Trott Trott added test Issues and PRs related to the tests. lts-watch-v4.x labels Jan 31, 2016
for (var i = 0; i < chunkSize; i++) {
chunkSize[i] = i % 256;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure this is safe to remove? Obviously the chunkSize[i] part is wrong, but perhaps this should be kept and just replace chunkSize[i] with data[i] assuming that was the original intention?

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 think it's safe to remove because I don't think the contents of the buffer affects anything in the test. But regardless, I can see how failing to initialize the buffer contents doesn't pass the smell test even if those contents are unused. So I'll put some initialization back.

It's odd because the modulus operator is superfluous because the buffer has a length of 250 so % 256 doesn't do anything.

And all this stuff was in the very first commit for the test. So it seems likely that this test has never run successfully.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, initialization restored.

@mscdex
Copy link
Contributor

mscdex commented Jan 31, 2016

LGTM if CI is happy: https://ci.nodejs.org/job/node-test-pull-request/1457/

@jasnell
Copy link
Member

jasnell commented Feb 1, 2016

LGTM

jasnell pushed a commit that referenced this pull request Feb 1, 2016
This fixes some variable redeclarations in pummel tests and, in the
process, gets `test-stream-pipe-multi.js` to run again. It had been
failing to run.

PR-URL: #4998
Reviewed-By: Brian White <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Feb 1, 2016

Landed in 39682b4

@jasnell jasnell closed this Feb 1, 2016
rvagg pushed a commit that referenced this pull request Feb 8, 2016
This fixes some variable redeclarations in pummel tests and, in the
process, gets `test-stream-pipe-multi.js` to run again. It had been
failing to run.

PR-URL: #4998
Reviewed-By: Brian White <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
This fixes some variable redeclarations in pummel tests and, in the
process, gets `test-stream-pipe-multi.js` to run again. It had been
failing to run.

PR-URL: #4998
Reviewed-By: Brian White <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
This fixes some variable redeclarations in pummel tests and, in the
process, gets `test-stream-pipe-multi.js` to run again. It had been
failing to run.

PR-URL: #4998
Reviewed-By: Brian White <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 18, 2016
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
This fixes some variable redeclarations in pummel tests and, in the
process, gets `test-stream-pipe-multi.js` to run again. It had been
failing to run.

PR-URL: #4998
Reviewed-By: Brian White <[email protected]>
Reviewed-By: James M Snell <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
This fixes some variable redeclarations in pummel tests and, in the
process, gets `test-stream-pipe-multi.js` to run again. It had been
failing to run.

PR-URL: nodejs#4998
Reviewed-By: Brian White <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott Trott deleted the pummel-no-redeclare branch January 13, 2022 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants