GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: simplify test-buffer-slice.js by starkwang · Pull Request #17962 · nodejs/node

Conversation

@starkwang

@starkwang starkwang commented Jan 3, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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 Node.js core tests and test infrastructure. label Jan 3, 2018
@starkwang

starkwang commented Jan 3, 2018

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax approved these changes Jan 3, 2018
View reviewed changes
gireeshpunathil
gireeshpunathil approved these changes Jan 3, 2018
View reviewed changes
cjihrig
cjihrig approved these changes Jan 3, 2018
View reviewed changes
maclover7
maclover7 approved these changes Jan 3, 2018
View reviewed changes
jasnell
jasnell approved these changes Jan 3, 2018
View reviewed changes
lpinca
lpinca approved these changes Jan 3, 2018
View reviewed changes
Trott
Trott reviewed Jan 4, 2018
View reviewed changes
Comment thread test/parallel/test-buffer-slice.js Outdated
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) {

@Trott Trott Jan 4, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@starkwang starkwang Jan 4, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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 ?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Trott Trott Jan 4, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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()?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@joyeecheung joyeecheung Jan 4, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@starkwang starkwang Jan 4, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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().

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Trott reacted with hooray emoji
All reactions
  • 🎉 1 reaction
@starkwang
[squash]change into buf.toString()
a914fbf
@starkwang
starkwang force-pushed the simplify-buffer-test branch from 2e7498a to a914fbf Compare January 4, 2018 06:05
BridgeAR
BridgeAR approved these changes Jan 4, 2018
View reviewed changes
@BridgeAR BridgeAR added fast-track PRs proposed for a shorter-than-standard waiting period before landing. author ready PRs with CI started, the required approvals, and no outstanding review comments. labels Jan 4, 2018
@BridgeAR

BridgeAR commented Jan 4, 2018

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@BridgeAR

BridgeAR commented Jan 5, 2018

Copy link
Copy Markdown
Member

Landed in a2d0623
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
v9.4.0 proposal #18069
Merged
@TimothyGu TimothyGu removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. 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 MylesBorins added land-on-v6.x labels Jan 24, 2018
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
v6.13.0 proposal #18342
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

@Trott Trott Trott left review comments
@joyeecheung joyeecheung joyeecheung left review comments
@jasnell jasnell jasnell approved these changes
@addaleax addaleax addaleax approved these changes
@lpinca lpinca lpinca approved these changes
@cjihrig cjihrig cjihrig approved these changes
@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes
@BridgeAR BridgeAR BridgeAR approved these changes
+1 more reviewer
@maclover7 maclover7 maclover7 approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

fast-track PRs proposed for a shorter-than-standard waiting period before landing. test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

13 participants

@starkwang @BridgeAR @jasnell @Trott @addaleax @lpinca @cjihrig @maclover7 @joyeecheung @gireeshpunathil @MylesBorins @TimothyGu @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.