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: fix test for buffer regression #649 #9924

Closed
wants to merge 3 commits into from
Closed

test: fix test for buffer regression #649 #9924

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d6a4e3d
test: fix test for buffer regression #649
joyeecheung Dec 1, 2016
1277c6a
test: squash line for buffer regression #649 test
joyeecheung Dec 4, 2016
c679654
test: loose the error message regexp for platform differences
joyeecheung Dec 6, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions test/parallel/test-buffer-regression-649.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,14 @@ const SlowBuffer = require('buffer').SlowBuffer;

// Regression test for https://github.com/nodejs/node/issues/649.
const len = 1422561062959;
assert.throws(() => Buffer(len).toString('utf8'));
assert.throws(() => SlowBuffer(len).toString('utf8'));
assert.throws(() => Buffer.alloc(len).toString('utf8'));
assert.throws(() => Buffer.allocUnsafe(len).toString('utf8'));
assert.throws(() => Buffer.allocUnsafeSlow(len).toString('utf8'));
const lenLimitMsg = /^RangeError: Invalid typed array length$/;
assert.throws(() => Buffer(len).toString('utf8'),
lenLimitMsg);
Copy link
Contributor

@cjihrig cjihrig Dec 4, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

All, or at least some, of these should fit on one line now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed.

A noob question: is it better that I amend the commits myself during the PR so you don't have to squash them when you land them, or is it better I just leave several commits in the PR so the review process won't get hidden in GitHub?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's really a personal preference. Sometimes with large PRs it's easier for the reviewer(s) if the entire thing doesn't have to be re-reviewed after each amended change. It can be tricky though if a PR contains multiple logically divided commits and you want to amend the correct commit. For something of this size, I don't think it matters much.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it, thanks for the explanation!

assert.throws(() => SlowBuffer(len).toString('utf8'),
lenLimitMsg);
assert.throws(() => Buffer.alloc(len).toString('utf8'),
lenLimitMsg);
assert.throws(() => Buffer.allocUnsafe(len).toString('utf8'),
lenLimitMsg);
assert.throws(() => Buffer.allocUnsafeSlow(len).toString('utf8'),
lenLimitMsg);