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: simplify test-buffer-slice.js #17962

Closed
wants to merge 2 commits into from
Closed

test: simplify test-buffer-slice.js #17962

wants to merge 2 commits into from

Conversation

starkwang
Copy link
Contributor

@starkwang starkwang commented Jan 3, 2018 •
edited
Loading

Use forEach loop to reduce some redundant codes.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

@starkwang
test: simplify test-buffer-slice.js
973e27e 
Use forEach loop to reduce some redundant codes.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 3, 2018
@starkwang
Copy link
Contributor Author

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

Trott
Trott reviewed Jan 4, 2018
View reviewed changes
test/parallel/test-buffer-slice.js Outdated
assert.strictEqual(0, Buffer.compare(buf.slice(-5, -3),
Buffer.from('56', 'utf8')));
assert.strictEqual(0, Buffer.compare(buf.slice(-10, 10),
Buffer.from('0123456789', 'utf8')));
for (let i = 0, s = buf; i < buf.length; ++i) {
Copy link
Member

Choose a reason for hiding this comment

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

I know this wasn't introduced by this change, but this segment of the test seems broken to me. s = buf means that they are pointing to the same buffer in memory, so of course buf.slice(i) and s.slice(i) are going to be the same thing. Maybe that's the point of the test? But I suspect not. Anyone understand this? Worth just removing this entire block? If not, it seems like it could use a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This segment was imported in this commit by @jasnell a year ago. I'm also confused with it.

@jasnell Could you tell us something about it ?

Copy link
Member

Choose a reason for hiding this comment

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

@starkwang I don't think that's the commit that introduced it. It's s = buf.toString() in that commit, which is very different from s = buf.

It appears that it was introduced in 02a4c08 and that it was an error. Does it make sense to add a second commit here or else in a different PR to restore it to buf.toString()?

Copy link
Member

Choose a reason for hiding this comment

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

@Trott I believe it should be buf.toString() as well, the s probably means "string" here (would be clearer if it gets renamed to str). Fixing it here SGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll change it into buf.toString().

@BridgeAR BridgeAR added fast-track PRs that do not need to wait for 48 hours to land. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jan 4, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Jan 4, 2018

Minimum CI to test the change https://ci.nodejs.org/job/node-test-commit-light/95/

@BridgeAR
Copy link
Member

BridgeAR commented Jan 5, 2018

Landed in a2d0623

@BridgeAR BridgeAR closed this Jan 5, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 5, 2018
@starkwang @BridgeAR
test: simplify test-buffer-slice.js
a2d0623 
Use forEach loop to reduce some redundant codes.

PR-URL: nodejs#17962
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@starkwang @MylesBorins
test: simplify test-buffer-slice.js
4074e52 
Use forEach loop to reduce some redundant codes.

PR-URL: #17962
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@starkwang @MylesBorins
test: simplify test-buffer-slice.js
0bfb7f6 
Use forEach loop to reduce some redundant codes.

PR-URL: #17962
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@starkwang @MylesBorins
test: simplify test-buffer-slice.js
f87a1a6 
Use forEach loop to reduce some redundant codes.

PR-URL: #17962
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
Merged
@TimothyGu TimothyGu removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 13, 2018
MylesBorins pushed a commit that referenced this pull request Jan 24, 2018
@starkwang @MylesBorins
test: simplify test-buffer-slice.js
9c1390d 
Use forEach loop to reduce some redundant codes.

PR-URL: #17962
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2018
@starkwang @MylesBorins
test: simplify test-buffer-slice.js
8e38ad9 
Use forEach loop to reduce some redundant codes.

PR-URL: #17962
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
@starkwang @MylesBorins
test: simplify test-buffer-slice.js
dbfb7d3 
Use forEach loop to reduce some redundant codes.

PR-URL: #17962
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
@starkwang @MylesBorins
test: simplify test-buffer-slice.js
1b86b0a 
Use forEach loop to reduce some redundant codes.

PR-URL: #17962
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
@starkwang @MylesBorins
test: simplify test-buffer-slice.js
1c4aa61 
Use forEach loop to reduce some redundant codes.

PR-URL: #17962
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joyeecheung joyeecheung joyeecheung left review comments

@Trott Trott Trott left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@maclover7 maclover7 maclover7 approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
fast-track PRs that do not need to wait for 48 hours to land. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.