Skip to content

test: testing noAssert in buffer.write() - #10790

Closed
larissayvette wants to merge 1 commit into
nodejs:masterfrom
larissayvette:test-write
Closed

test: testing noAssert in buffer.write()#10790
larissayvette wants to merge 1 commit into
nodejs:masterfrom
larissayvette:test-write

Conversation

@larissayvette

@larissayvettelarissayvette commented Jan 13, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor
Checklist
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Test for noAssert in buffer.write() files and inluded buffer.writeDouble* and buffer.writeFloat* as it was not present in the first commit

@nodejs-github-botnodejs-github-bot added test Issues and PRs related to the tests. lts-watch-v6.x labels Jan 13, 2017
@mscdexmscdex added the buffer Issues and PRs related to the buffer subsystem. label Jan 13, 2017
@Trott

Trott commented Jan 13, 2017

Copy link
Copy Markdown
Member

buffer.writeDouble* and buffer.writeFloat*

You should be able to run these tests for those functions as well. The issue with those functions will only manifest if you request something out of range. Since this test requests valid ranges with noAssert enabled, it should be OK.

@larissayvette

larissayvette commented Jan 14, 2017

Copy link
Copy Markdown
ContributorAuthor

@Trott included those test cases buffer.writeDouble* and buffer.writeFloat*

Trott
Trott approved these changes Jan 14, 2017
View reviewed changes

@TrottTrott 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 if CI is green

@Trott

Trott commented Jan 14, 2017

Copy link
Copy Markdown
Member

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

thefourtheye
thefourtheye reviewed Jan 14, 2017
View reviewed changes
Comment threadtest/parallel/test-buffer-write-noassert.js Outdated

@thefourtheyethefourtheyeJan 14, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This functionality is tested every time. Wouldn't it be enough to test this only once?

@TrottTrottJan 14, 2017

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.

Oh, yes, that's testing the wrong thing. It should test a legit alloc() call and an out-of-range call to the write function being tested. @larissayvette can you adjust it?

@Trott

Trott commented Jan 26, 2017

Copy link
Copy Markdown
Member

Changes LGTM

@Trott

Trott commented Jan 26, 2017

Copy link
Copy Markdown
Member

New CI: https://ci.nodejs.org/job/node-test-pull-request/6075/

@Trott

Trott commented Jan 26, 2017

Copy link
Copy Markdown
Member

Single failure on SmartOS is unrelated, opened an issue for it.

Trott pushed a commit to Trott/io.js that referenced this pull request Jan 26, 2017
@larissayvette@Trott
test: check noAssert option in buf.write*()
084acc8
Add test to cover previously untested `noAssert` functionality in
buf.write*() functions.
PR-URL: nodejs#10790
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott

Trott commented Jan 26, 2017

Copy link
Copy Markdown
Member

Landed in 084acc8.
Thanks for the contribution! 🎉

@TrottTrott closed this Jan 26, 2017
evanlucas pushed a commit that referenced this pull request Jan 31, 2017
@larissayvette@evanlucas
test: check noAssert option in buf.write*()
c1e166a
Add test to cover previously untested `noAssert` functionality in
buf.write*() functions.
PR-URL: #10790
Reviewed-By: Rich Trott <rtrott@gmail.com>
@italoacasasitaloacasas mentioned this pull request Jan 31, 2017
Merged
@larissayvette
larissayvette deleted the test-write branch February 10, 2017 17:42
jasnell pushed a commit that referenced this pull request Mar 8, 2017
@larissayvette@jasnell
test: check noAssert option in buf.write*()
a9acc2f
Add test to cover previously untested `noAssert` functionality in
buf.write*() functions.
PR-URL: #10790
Reviewed-By: Rich Trott <rtrott@gmail.com>
@jasnell

jasnell commented Mar 8, 2017

Copy link
Copy Markdown
Member

This would need a backport PR to land in v4

MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@larissayvette@MylesBorins
test: check noAssert option in buf.write*()
a175186
Add test to cover previously untested `noAssert` functionality in
buf.write*() functions.
PR-URL: #10790
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Mar 9, 2017
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TrottTrottTrott approved these changes
+1 more reviewer
@thefourtheyethefourtheyethefourtheye left review comments
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.

6 participants

@larissayvette@Trott@jasnell@thefourtheye@mscdex@nodejs-github-bot