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

Adds assertions to zero length buffer slice test #8729

Closed
wants to merge 1 commit into from
Closed

Adds assertions to zero length buffer slice test #8729

wants to merge 1 commit into from

Conversation

LaurenSpiegel
Copy link
Contributor

Checklist
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

buffer

Description of change

Existing test to show that slicing a 0 length buffer does not throw was missing the actual doesNotThrow assertion.

  1. Adds missing assertion that slicing a 0 length buffer does not throw.
  2. Adds assertion that slicing a 0 length buffer has a length of 0.

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 23, 2016
@mscdex mscdex added the buffer Issues and PRs related to the buffer subsystem. label Sep 23, 2016
@lpinca
Copy link
Member

lpinca commented Sep 23, 2016

I guess the reason to not use assert.doesNotThrow() was that a thrown error would make the test fail anyway.
That said the change LGTM. Can you please update the commit subject line and make it follow the guidelines? Thanks!

@LaurenSpiegel
Copy link
Contributor Author

Thanks, @lpinca! Updated.

lpinca
lpinca approved these changes Sep 23, 2016
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM

addaleax
addaleax approved these changes Sep 23, 2016
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

CI: https://ci.nodejs.org/job/node-test-commit/5273/

jasnell
jasnell approved these changes Sep 23, 2016
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM

imyller
imyller approved these changes Sep 23, 2016
View reviewed changes
Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

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

LGTM

@LaurenSpiegel
Copy link
Contributor Author

Hi, the CI failures do not seem related to the changes in the PR. Any insights?

thefourtheye
thefourtheye approved these changes Sep 24, 2016
View reviewed changes
Copy link
Contributor

@thefourtheye thefourtheye left a comment

Choose a reason for hiding this comment

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

LGTM

test/parallel/test-buffer-slice.js
@@ -52,7 +52,8 @@ assert.equal(buf.slice('0', '-111'), '');

// try to slice a zero length Buffer
// see https://github.com/joyent/node/issues/5881
Buffer.alloc(0).slice(0, 1);
assert.doesNotThrow(() => { Buffer.alloc(0).slice(0, 1) });
Copy link
Contributor

Choose a reason for hiding this comment

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

You can drop the block and simply write this as

assert.doesNotThrow(() => Buffer.alloc(0).slice(0, 1));

@lpinca
Copy link
Member

lpinca commented Sep 24, 2016

@LaurenSpiegel as you said CI failures seem to not be related to this change. Nothing to worry about IMHO.
Starting a new build: https://ci.nodejs.org/job/node-test-pull-request/4254/

@lpinca
Copy link
Member

lpinca commented Sep 24, 2016 •
edited
Loading

There is actually a lint error, a missing semicolon on the first assertion. If you follow @thefourtheye's suggestion that will be fixed.
You can run make jslint to check for lint errors.

@LaurenSpiegel
test: add assertions to zero length buffer test
ef85c9f 
1) Add missing assertion that slicing a 0 length buffer does not throw
2) Add assertion that slicing a 0 length buffer has a length of 0
@LaurenSpiegel
Copy link
Contributor Author

Thanks, lint error fixed and @thefourtheye's change incorporated.

@thefourtheye
Copy link
Contributor

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

jasnell pushed a commit that referenced this pull request Sep 26, 2016
@LaurenSpiegel @jasnell
test: add assertions to zero length buffer test
927661f 
1) Add missing assertion that slicing a 0 length buffer does not throw
2) Add assertion that slicing a 0 length buffer has a length of 0

PR-URL: #8729
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@jasnell
Copy link
Member

jasnell commented Sep 26, 2016

Landed in 927661f.

@jasnell jasnell closed this Sep 26, 2016
jasnell pushed a commit that referenced this pull request Sep 29, 2016
@LaurenSpiegel @jasnell
test: add assertions to zero length buffer test
5653772 
1) Add missing assertion that slicing a 0 length buffer does not throw
2) Add assertion that slicing a 0 length buffer has a length of 0

PR-URL: #8729
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
@LaurenSpiegel @Fishrock123
test: add assertions to zero length buffer test
81ed50c 
1) Add missing assertion that slicing a 0 length buffer does not throw
2) Add assertion that slicing a 0 length buffer has a length of 0

PR-URL: #8729
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@thefourtheye thefourtheye thefourtheye approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

@imyller imyller imyller approved these changes

Assignees
No one assigned
Labels
buffer Issues and PRs related to the buffer 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.
9 participants
@LaurenSpiegel @lpinca @thefourtheye @jasnell @addaleax @imyller @mscdex @MylesBorins @nodejs-github-bot