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, bechmark: use new Buffer API where appropriate #18980

Closed
wants to merge 1 commit into from
Closed

test, bechmark: use new Buffer API where appropriate #18980

wants to merge 1 commit into from

Conversation

ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented Feb 24, 2018

For tests / benchmarks that are creating Buffer instances for any reason other than to test Buffer constructor, use the new Buffer.alloc/Buffer.from API instead of the deprecated API.

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)

test, benchmark

@ChALkeR ChALkeR added test Issues and PRs related to the tests. benchmark Issues and PRs related to the benchmark subsystem. labels Feb 24, 2018
@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. benchmark Issues and PRs related to the benchmark subsystem. stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests. labels Feb 24, 2018
cjihrig
cjihrig approved these changes Feb 24, 2018
View reviewed changes
test/parallel/test-buffer-zero-fill.js Outdated
@@ -3,6 +3,7 @@
require('../common');
const assert = require('assert');

// Tests deprecated Buffer API on a purpose
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe write this as "Tests the deprecated Buffer() constructor"

If you don't change it, "on a purpose" should be "on purpose"

Copy link
Member Author

@ChALkeR ChALkeR Feb 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Technically, the first line of those two (Buffer(100)) is not a constructor call by itself, it's the commit description which is a bit inaccurate there (but rewording will make it needlessly longer).

I will fix the language though, thanks!

@ChALkeR
test,bechmark: use new Buffer API where appropriate
bc15526 
For tests / benchmarks that are creating Buffer instances for
any reason other than to test Buffer constructor, use the new
Buffer.alloc/Buffer.from API instead of the deprecated API.
@ChALkeR ChALkeR force-pushed the buffer-usage-cleanup branch from be71228 to bc15526 Compare February 24, 2018 18:12
mhdawson
mhdawson approved these changes Mar 1, 2018
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

BridgeAR
BridgeAR approved these changes Mar 1, 2018
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM. We might actually introduce a linting rule so it does not sneak back in at some point.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 1, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Mar 1, 2018

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

@ChALkeR please always start a CI after opening a PR :-)

@apapirovski
Copy link
Member

Landed in 8ed44ff

@apapirovski apapirovski closed this Mar 4, 2018
apapirovski pushed a commit that referenced this pull request Mar 4, 2018
@ChALkeR @apapirovski
test,benchmark: use new Buffer API where appropriate
8ed44ff 
For tests / benchmarks that are creating Buffer instances for
any reason other than to test Buffer constructor, use the new
Buffer.alloc/Buffer.from API instead of the deprecated API.

PR-URL: #18980
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this pull request Mar 5, 2018
@ChALkeR @addaleax
test,benchmark: use new Buffer API where appropriate
f864509 
For tests / benchmarks that are creating Buffer instances for
any reason other than to test Buffer constructor, use the new
Buffer.alloc/Buffer.from API instead of the deprecated API.

PR-URL: nodejs#18980
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 6, 2018
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@ChALkeR @MayaLekova
test,benchmark: use new Buffer API where appropriate
11d0991 
For tests / benchmarks that are creating Buffer instances for
any reason other than to test Buffer constructor, use the new
Buffer.alloc/Buffer.from API instead of the deprecated API.

PR-URL: nodejs#18980
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
@ChALkeR ChALkeR deleted the buffer-usage-cleanup branch July 9, 2018 13:49
@jasnell
Copy link
Member

jasnell commented Aug 17, 2018

If this needs to be backported to 8.x, a separate backport PR will be necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@tniessen tniessen tniessen approved these changes

@cjihrig cjihrig cjihrig approved these changes

@watilde watilde watilde approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
async_hooks Issues and PRs related to the async hooks subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. benchmark Issues and PRs related to the benchmark subsystem. stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@ChALkeR @BridgeAR @apapirovski @jasnell @lpinca @watilde @cjihrig @tniessen @mhdawson @nodejs-github-bot