test: buffer should always be stringified by lucamaraschi · Pull Request #12355 · nodejs/node

lucamaraschi · 2017-04-12T09:39:13Z

This test makes sure that independently of the buffer type, the input
is always stringify and generates a valid input.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

test fs

vsemozhetbyt · 2017-04-12T09:51:21Z

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

vsemozhetbyt · 2017-04-12T09:52:12Z

test/parallel/test-fs-buffertype-writesync.js

BTW, why 'a', not 'w'?

Perhaps we can test with all the flags.

I think testing with all flags is an overhead as we are not testing for the fd but for the buffer type. @vsemozhetbyt and @thefourtheye what do u think about it?

I would suggest moving the common.refreshTmpDir() out of the loop and using 'w' here, that’s much cleaner.

thefourtheye · 2017-04-12T11:08:06Z

test/parallel/test-fs-buffertype-writesync.js

Nit: I think ensures will be more suitable than insures

Ooops...good catch!

Also, I don’t quite understand the statement here – you talk about input buffers for writeSync, but the only time you are actually using buffers is in the readFileSync output?

^ I think this comment is still unaddressed? It’s just a comment but it’s really a bit confusing.

@addalex I amended the comment here.

addaleax · 2017-04-12T11:46:00Z

test/parallel/test-fs-buffertype-writesync.js

I would suggest moving the common.refreshTmpDir() out of the loop and using 'w' here, that’s much cleaner.

addaleax · 2017-04-12T11:47:10Z

test/parallel/test-fs-buffertype-writesync.js

Also, I don’t quite understand the statement here – you talk about input buffers for writeSync, but the only time you are actually using buffers is in the readFileSync output?

cjihrig

LGTM with @addaleax's comment addressed. Might want to rename v to something more descriptive like values too.

thefourtheye · 2017-04-13T07:28:13Z

test/parallel/test-fs-buffertype-writesync.js

This removes and creates the temporary directory, so this should be before the tests.

This test makes sure that independently of the buffer type, the input is always stringified and generates a valid input.

lucamaraschi · 2017-04-17T23:14:17Z

@addaleax and @thefourtheye does my last change make sense to you?

thefourtheye

Yes, LGTM

addaleax

right, sorry – LGTM!

cjihrig · 2017-04-18T12:50:45Z

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

jasnell · 2017-04-18T14:50:54Z

CI failure on Windows looks unrelated.

This test makes sure that independently of the buffer type, the input is always stringified and generates a valid input. PR-URL: #12355 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

jasnell · 2017-04-18T18:07:47Z

Landed in 9e26347

This test makes sure that independently of the buffer type, the input is always stringified and generates a valid input. PR-URL: #12355 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

gibfahn · 2017-05-16T12:26:14Z

Landed on v6.x, LMK if it shouldn't have.

This test makes sure that independently of the buffer type, the input is always stringified and generates a valid input. PR-URL: #12355 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

This test makes sure that independently of the buffer type, the input is always stringified and generates a valid input. PR-URL: nodejs/node#12355 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 12, 2017

vsemozhetbyt approved these changes Apr 12, 2017

View reviewed changes

vsemozhetbyt reviewed Apr 12, 2017

View reviewed changes

vsemozhetbyt added the buffer Issues and PRs related to the buffer subsystem. label Apr 12, 2017

jasnell approved these changes Apr 12, 2017

View reviewed changes

thefourtheye reviewed Apr 12, 2017

View reviewed changes

lucamaraschi force-pushed the test-fs-buffertype-writeSync branch from af6825b to e6d70ae Compare April 12, 2017 11:47

addaleax requested changes Apr 12, 2017

View reviewed changes

cjihrig approved these changes Apr 12, 2017

View reviewed changes

lucamaraschi force-pushed the test-fs-buffertype-writeSync branch from e6d70ae to b5db315 Compare April 12, 2017 13:18

mscdex added fs Issues and PRs related to the fs subsystem / file system. and removed buffer Issues and PRs related to the buffer subsystem. labels Apr 12, 2017

thefourtheye suggested changes Apr 13, 2017

View reviewed changes

test: buffer should always be stringified

38c76ee

This test makes sure that independently of the buffer type, the input is always stringified and generates a valid input.

lucamaraschi force-pushed the test-fs-buffertype-writeSync branch from b5db315 to 38c76ee Compare April 13, 2017 08:10

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:12

thefourtheye approved these changes Apr 18, 2017

View reviewed changes

addaleax approved these changes Apr 18, 2017

View reviewed changes

jasnell closed this Apr 18, 2017

evanlucas mentioned this pull request May 1, 2017

v7.10.0 proposal #12775

Merged

gibfahn added the land-on-v6.x label May 16, 2017

MylesBorins mentioned this pull request May 23, 2017

v6.11.0 proposal #13059

Merged

Uh oh!

Conversation

lucamaraschi commented Apr 12, 2017

Checklist

Affected core subsystem(s)

Uh oh!

vsemozhetbyt commented Apr 12, 2017

Uh oh!

vsemozhetbyt Apr 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucamaraschi commented Apr 17, 2017

Uh oh!

thefourtheye left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Apr 18, 2017

Uh oh!

jasnell commented Apr 18, 2017

Uh oh!

jasnell commented Apr 18, 2017

Uh oh!

gibfahn commented May 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

vsemozhetbyt Apr 12, 2017 •

edited

Loading