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.

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

test: refactor buffer tests #8256

Closed
wants to merge 12 commits into from
Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Aug 24, 2016

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

buffer,test

Description of change

Refactor the Buffer tests, eliminate duplication, move certain tests out to the own test files, updated and modernize, prefer use of strictEqual where appropriate.

@jasnell jasnell added buffer Issues and PRs related to the buffer subsystem. test Issues and PRs related to the tests. labels Aug 24, 2016
@jasnell
Copy link
Member Author

jasnell commented Aug 24, 2016

for (const size of sizes) {
try {
// These allocations are known to fail. If they do,
// Uin32Array should still produce a zeroed out result.
Copy link
Member

Choose a reason for hiding this comment

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

typo: Uint32Array

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

@jasnell jasnell force-pushed the refactor-test-buffer branch from a9e1407 to d67bd34 Compare August 24, 2016 17:54
@targos
Copy link
Member

targos commented Aug 24, 2016

LGTM. Thanks for keeping a lot of separate commits!

var b = Buffer.allocUnsafe(1024);

console.log('b.length == %d', b.length);
const b = Buffer.allocUnsafe(1024);
assert.strictEqual(1024, b.length);
Copy link
Member

Choose a reason for hiding this comment

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

The API for assert.strictEqual() and friends indicates that the actual value received should be the first parameter and the expected value should be second but it's backwards here and below. Feel free to leave it as is (since this isn't code you wrote, it's just an adjacent line), but if you want to swap them, hey, go for it.

Copy link
Member

Choose a reason for hiding this comment

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

(And yes, I'm going for the most useless nit I can imagine here. Like I said, feel free to ignore.)

Copy link
Member Author

Choose a reason for hiding this comment

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

There are many places in the tests where they are in the wrong order. For now, I'm inclined to leave these as they are. It would be a good first contribution for someone tho :-)

jasnell added a commit that referenced this pull request Aug 26, 2016
Remove duplication of buffer tests, separate out into separate
files, update and cleanup code, move to using strictEqual where
possible.

PR-URL: #8256
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Aug 26, 2016

Landed in 7053922!

@jasnell jasnell closed this Aug 26, 2016
@Fishrock123 Fishrock123 mentioned this pull request Sep 6, 2016
@Fishrock123
Copy link
Contributor

Having lots of conflicts on this into v6.

Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Sep 8, 2016
Remove duplication of buffer tests, separate out into separate
files, update and cleanup code, move to using strictEqual where
possible.

PR-URL: nodejs#8256
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

 Conflicts:
	test/parallel/test-buffer-alloc.js
	test/parallel/test-buffer.js
	test/parallel/test-buffer-no-negative-allocation.js
Fishrock123 pushed a commit that referenced this pull request Sep 9, 2016
Remove duplication of buffer tests, separate out into separate
files, update and cleanup code, move to using strictEqual where
possible.

PR-URL: #8256
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

 Conflicts:
	test/parallel/test-buffer-alloc.js
	test/parallel/test-buffer.js
	test/parallel/test-buffer-no-negative-allocation.js
@MylesBorins
Copy link
Contributor

this is not landing on v4.x please feel free to backport

@targos targos mentioned this pull request Oct 7, 2016
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants