Skip to content

test: increase coverage of buffer - #11122

Closed
DavidCai1111 wants to merge 1 commit into
nodejs:masterfrom
DavidCai1111:master
Closed

test: increase coverage of buffer#11122
DavidCai1111 wants to merge 1 commit into
nodejs:masterfrom
DavidCai1111:master

Conversation

@DavidCai1111

@DavidCai1111DavidCai1111 commented Feb 2, 2017

Copy link
Copy Markdown
Member
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

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Feb 2, 2017
@hiroppyhiroppy added the buffer Issues and PRs related to the buffer subsystem. label Feb 2, 2017
joyeecheung
joyeecheung approved these changes Feb 2, 2017
View reviewed changes
Comment threadtest/parallel/test-buffer-write-noassert.js Outdated

@joyeecheungjoyeecheungFeb 2, 2017•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Something like

consterror=/Int/.test(funx) ?
/^TypeError:"buffer"argumentmustbeaBufferorUint8Array$/ :
/^TypeError:argumentshouldbeaBuffer$/;assert.throws(()=>Buffer.alloc(9)[funx].apply(newUint32Array(1),args),error);

would probably be easier to read.

jasnell
jasnell approved these changes Feb 2, 2017
View reviewed changes

@jasnelljasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM with @joyeecheung's formatting suggestion

@DavidCai1111

DavidCai1111 commented Feb 3, 2017

Copy link
Copy Markdown
MemberAuthor

OK, updated.

@italoacasas

italoacasas commented Feb 3, 2017

Copy link
Copy Markdown

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

jasnell pushed a commit that referenced this pull request Feb 6, 2017
@davidtaikocha@jasnell
test: increase coverage of buffer
901cb8c
PR-URL: #11122
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jasnell

jasnell commented Feb 6, 2017

Copy link
Copy Markdown
Member

Landed in 901cb8c

@jasnelljasnell closed this Feb 6, 2017
@italoacasas

italoacasas commented Feb 9, 2017

Copy link
Copy Markdown

This is failing in v7.x, maybe because of some semver-major in buffer. Any plan to backport this?

@joyeecheung

joyeecheung commented Feb 9, 2017•
edited
Loading

Copy link
Copy Markdown
Member

This depends on semver-major #10236 which adds support for Uint8Array input to buffer methods. Should be manually backportable though. @DavidCai1993 mind submitting a backport for this?

@DavidCai1111

DavidCai1111 commented Feb 10, 2017

Copy link
Copy Markdown
MemberAuthor

OK

@DavidCai1111DavidCai1111 mentioned this pull request Feb 11, 2017
Closed
3 tasks
italoacasas pushed a commit that referenced this pull request Feb 16, 2017
@davidtaikochaItalo A. Casas
test: increase coverage of buffer
f542c59
PR-URL: #11312
Backport-of: #11122
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
italoacasas pushed a commit that referenced this pull request Feb 22, 2017
@davidtaikochaItalo A. Casas
test: increase coverage of buffer
66081d1
PR-URL: #11312
Backport-of: #11122
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
krydos pushed a commit to krydos/node that referenced this pull request Feb 25, 2017
@davidtaikocha@krydos
test: increase coverage of buffer
54e2f1e
PR-URL: nodejs#11122
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jasnelljasnell mentioned this pull request Apr 4, 2017
Closed
@gibfahngibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
@gibfahn

gibfahn commented Jun 17, 2017

Copy link
Copy Markdown
Member

Adding dont-land-on-v6.x due to #11312 (comment), @DavidCai1993 if you think this should be backported please raise a PR (see the backporting guide for the howto.

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

Reviewers

@jasnelljasnelljasnell approved these changes
@joyeecheungjoyeecheungjoyeecheung approved these changes
+3 more reviewers
@thefourtheyethefourtheyethefourtheye approved these changes
@hiroppyhiroppyhiroppy approved these changes
@italoacasasitaloacasasitaloacasas approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bufferIssues and PRs related to the buffer subsystem.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@DavidCai1111@italoacasas@jasnell@joyeecheung@gibfahn@thefourtheye@hiroppy@nodejs-github-bot@davidtaikocha