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

Update buffer swap test to use better variable names #12544

Closed
wants to merge 1 commit into from

Conversation

neeharv
Copy link
Contributor

@neeharv neeharv commented Apr 20, 2017

tests/parallel/buffer-swap.js has all variables defined at the top level scope, and hence the variable names are variations of buf, buf2, buf3 etc.

This PR puts each setup/assertion code block into its own block, and renames the variables to be easier to read.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 20, 2017
@neeharv
Copy link
Contributor Author

neeharv commented Apr 20, 2017

cc @Trott

@Fishrock123
Copy link
Contributor

0x03, 0x04, 0x05, 0x06, 0x07, 0x08,
0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e,
0x0f, 0x10]));
let commonBuf;
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the point of moving to block scoping to avoid shared variables like this?

@mscdex
Copy link
Contributor

mscdex commented Apr 20, 2017

How about either moving the commonBuf usage at the bottom of the file into the area where commonBuf is set or move the code where commonBuf is set to the bottom of the file, remove its surrounding braces, and rename commonBuf to buf or something ('commonBuf' does not seem like a good name/description).

@mscdex mscdex added the buffer Issues and PRs related to the buffer subsystem. label Apr 20, 2017
@neeharv
Copy link
Contributor Author

neeharv commented Apr 20, 2017

@mscdex @cjihrig I wasn't sure of moving the test code around too much, but what you're saying makes sense. I've moved the length assertions into the block where the relevant buffer variable was being defined.

@neeharv
Copy link
Contributor Author

neeharv commented Apr 21, 2017

Not sure why CI failed. All tests pass fine locally (OS X), looks like the fedora build failing was because I force pushed some changes while the tests were running.

@mscdex
Copy link
Contributor

mscdex commented Apr 21, 2017

@Trott
Copy link
Member

Trott commented Apr 21, 2017

Not sure why CI failed.

The two CI failures in the previous run look like build failures unrelated to this test change. Hopefully the newer CI run will come back all green.

jasnell pushed a commit that referenced this pull request Apr 24, 2017
PR-URL: #12544
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Apr 24, 2017

Landed in 5096651

@jasnell jasnell closed this Apr 24, 2017
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
PR-URL: #12544
Reviewed-By: James M Snell <jasnell@gmail.com>
@evanlucas evanlucas mentioned this pull request May 1, 2017
evanlucas pushed a commit that referenced this pull request May 1, 2017
PR-URL: #12544
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas pushed a commit that referenced this pull request May 2, 2017
PR-URL: #12544
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this pull request May 16, 2017
PR-URL: #12544
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 18, 2017
PR-URL: #12544
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 23, 2017
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
PR-URL: nodejs/node#12544
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants