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: complete coverage of buffer #12831

Closed
wants to merge 1 commit into from

Conversation

@DavidCai1993
Copy link
Member

commented May 4, 2017

Complete coverage of lib/buffer.js. According to https://coverage.nodejs.org/coverage-ff001c12b032c33d/root/buffer.js.html , this PR adds the coverage for some missing break cases and one utf-8 case which are not get covered in #12714 .

PS: When i was writing the test cases for those invalid encodings cases, i found that the way each Buffer API treats the invalid value of the encoding parameter seems quite inconsistent, for instance:

  • Buffer.from treats all JavaScript falsy values and all other non-string values as utf8
    • Buffer.from('foo', false) equals Buffer.from('foo', 'utf8')
    • Buffer.from('foo', 123) equals Buffer.from('foo', 'utf8')
  • buf.toString only treats undefined as utf8, and throws an error when comes to all other invalid values
    • Buffer.from('foo').toString(undefined) equals Buffer.from('foo').toString('utf8')
    • Buffer.from('foo').toString('u') and Buffer.from('foo').toString(0) will throw
  • buf.write just treats all JavaScript falsy value as utf8, and throws an error when comes to all other invalid values
    • Buffer.alloc(9).write('foo', 0) equals Buffer.alloc(9).write('foo', 'utf8')
  • ...

So shall we find a consistent way to deal with this invalid encoding parameter situation to avoid some potential confusions?

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

@aqrln
aqrln approved these changes May 4, 2017
test/parallel/test-buffer-tostring.js Outdated
@@ -23,3 +23,11 @@ encodings
assert.strictEqual(Buffer.from('666f6f', encoding).toString(encoding),
'666f6f');
});

// invalid encodings

This comment has been minimized.

Copy link
@aqrln

aqrln May 4, 2017

Member

I'd prefer the comment to start with a capital letter, but maybe it's just me, idk :)

test/parallel/test-buffer-write.js Outdated
@@ -53,3 +53,11 @@ encodings
assert.strictEqual(buf.write('666f6f', 0, len, encoding), len);
assert.deepStrictEqual(buf, resultMap.get(encoding.toLowerCase()));
});

// invalid encodings

This comment has been minimized.

Copy link
@aqrln

aqrln May 4, 2017

Member

Ditto.

@hiroppy
hiroppy approved these changes May 5, 2017
@cjihrig
cjihrig approved these changes May 5, 2017
Copy link
Contributor

left a comment

LGTM with a comment.

test/parallel/test-buffer-bytelength.js Outdated
for (let i = 1; i < 10; i++) {
const encoding = String(i).repeat(i);

assert.strictEqual(Buffer.byteLength('foo', encoding),

This comment has been minimized.

Copy link
@cjihrig

cjihrig May 5, 2017

Contributor

Could you use Buffer.isEncoding() here?

@DavidCai1993 DavidCai1993 force-pushed the DavidCai1993:buffer/coverage branch to 3202a99 May 5, 2017

@DavidCai1993

This comment has been minimized.

Copy link
Member Author

commented May 5, 2017

@aqrln @cjihrig Updated, PTAL :=)

@DavidCai1993

This comment has been minimized.

Copy link
Member Author

commented May 5, 2017

@cjihrig
cjihrig approved these changes May 5, 2017
@jasnell
jasnell approved these changes May 5, 2017
@addaleax

This comment has been minimized.

Copy link
Member

commented May 7, 2017

Landed in a710e44

@addaleax addaleax closed this May 7, 2017

addaleax added a commit that referenced this pull request May 7, 2017
test: complete coverage of buffer
PR-URL: #12831
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
anchnk pushed a commit to anchnk/node that referenced this pull request May 19, 2017
test: complete coverage of buffer
PR-URL: nodejs#12831
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@jasnell jasnell referenced this pull request May 11, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 20, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.