Skip to content

Conversation

@lucamaraschi
Copy link
Contributor

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

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Apr 12, 2017
@vsemozhetbyt
Copy link
Contributor

Copy link
Contributor

@vsemozhetbytvsemozhetbytApr 12, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we can test with all the flags.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

@vsemozhetbytvsemozhetbyt added the buffer Issues and PRs related to the buffer subsystem. label Apr 12, 2017
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think ensures will be more suitable than insures

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Ooops...good catch!

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@addalex I amended the comment here.

@lucamaraschilucamaraschiforce-pushed the test-fs-buffertype-writeSync branch from af6825b to e6d70aeCompareApril 12, 2017 11:47
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@cjihrigcjihrig left a comment

Choose a reason for hiding this comment

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

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

@lucamaraschilucamaraschiforce-pushed the test-fs-buffertype-writeSync branch from e6d70ae to b5db315CompareApril 12, 2017 13:18
@mscdexmscdex 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
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
ContributorAuthor

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

Copy link
Contributor

@thefourtheyethefourtheye left a comment

Choose a reason for hiding this comment

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

Yes, LGTM

Copy link
Member

@addaleaxaddaleax left a comment

Choose a reason for hiding this comment

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

right, sorry – LGTM!

@cjihrig
Copy link
Contributor

@jasnell
Copy link
Member

CI failure on Windows looks unrelated.

jasnell pushed a commit that referenced this pull request Apr 18, 2017
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@jasnell
Copy link
Member

Landed in 9e26347

@jasnelljasnell closed this Apr 18, 2017
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@evanlucasevanlucas mentioned this pull request May 1, 2017
evanlucas pushed a commit that referenced this pull request May 1, 2017
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
evanlucas pushed a commit that referenced this pull request May 2, 2017
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@gibfahn
Copy link
Member

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

gibfahn pushed a commit that referenced this pull request May 16, 2017
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 18, 2017
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request May 23, 2017
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fsIssues and PRs related to the fs subsystem / file system.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@lucamaraschi@vsemozhetbyt@cjihrig@jasnell@gibfahn@thefourtheye@addaleax@mscdex@nodejs-github-bot