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

test: decrease inconsistency in the common.js #7758

Closed
wants to merge 2 commits into from
Closed

test: decrease inconsistency in the common.js #7758

wants to merge 2 commits into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Jul 16, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change
  1. Several non-reassigned var are replaced by const (only in module top level, function top level, one block boundary).
  2. process.env['PROCESSOR_ARCHITEW6432'] -> process.env.PROCESSOR_ARCHITEW6432.
  3. path.dirname(__filename) -> __dirname.
  4. path.resolve(__dirname) -> __dirname.

I'm not sure about replacing two complicated string cocncatenations in the exports.ddCommand() by template strings. If it is a right thing, I could try to add a commit.

Sorry, if I have just added some newbie mess instead of fixing. I've tried to understand the common.js while doing my first test for Node.js, and stumbled on some fragments, so I've decided to suggest some possible small improvements.

1. Several non-reassigned `var` are replaced by `const`
   (only in module top evel, function top level, one block boundary).

2. `process.env['PROCESSOR_ARCHITEW6432']` ->
   `process.env.PROCESSOR_ARCHITEW6432`.
   
3. `path.dirname(__filename)` -> `__dirname`.

4. `path.resolve(__dirname)` -> `__dirname`.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 16, 2016
@Trott
Copy link
Member

Trott commented Jul 16, 2016

@Fishrock123
Copy link
Contributor

@vsemozhetbyt Could you remove the var to const renames? I'm ok with the other bits, but those bits are at high risk of creating merge conflicts with little benefit.

@vsemozhetbyt
Copy link
Contributor Author

@Fishrock123 Done.

@cjihrig
Copy link
Contributor

cjihrig commented Jul 18, 2016

LGTM

3 similar comments
@Fishrock123
Copy link
Contributor

LGTM

@addaleax
Copy link
Member

LGTM

@thefourtheye
Copy link
Contributor

LGTM

@addaleax
Copy link
Member

One more CI, will land if it’s green: https://ci.nodejs.org/job/node-test-commit/4287/

@addaleax
Copy link
Member

addaleax commented Aug 1, 2016

CI again, that last one failed with infrastructure problems and a known flaky test: https://ci.nodejs.org/job/node-test-commit/4347/

@addaleax
Copy link
Member

addaleax commented Aug 1, 2016

The last CI was green with the exception of a parallel/test-net-connect-local-error failure that doesn’t seem to be related.

Landed in 0de55d8, thank you for the PR!

@addaleax addaleax closed this Aug 1, 2016
addaleax pushed a commit that referenced this pull request Aug 1, 2016
1. `process.env['PROCESSOR_ARCHITEW6432']` ->
   `process.env.PROCESSOR_ARCHITEW6432`.

2. `path.dirname(__filename)` -> `__dirname`.

3. `path.resolve(__dirname)` -> `__dirname`.

PR-URL: #7758
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@vsemozhetbyt
Copy link
Contributor Author

@addaleax Thank you for your help and time.

@vsemozhetbyt vsemozhetbyt deleted the patch-1 branch August 1, 2016 12:36
@cjihrig cjihrig mentioned this pull request Aug 8, 2016
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
1. `process.env['PROCESSOR_ARCHITEW6432']` ->
   `process.env.PROCESSOR_ARCHITEW6432`.

2. `path.dirname(__filename)` -> `__dirname`.

3. `path.resolve(__dirname)` -> `__dirname`.

PR-URL: #7758
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
MylesBorins pushed a commit that referenced this pull request Sep 30, 2016
1. `process.env['PROCESSOR_ARCHITEW6432']` ->
   `process.env.PROCESSOR_ARCHITEW6432`.

2. `path.dirname(__filename)` -> `__dirname`.

3. `path.resolve(__dirname)` -> `__dirname`.

PR-URL: #7758
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
1. `process.env['PROCESSOR_ARCHITEW6432']` ->
   `process.env.PROCESSOR_ARCHITEW6432`.

2. `path.dirname(__filename)` -> `__dirname`.

3. `path.resolve(__dirname)` -> `__dirname`.

PR-URL: #7758
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
1. `process.env['PROCESSOR_ARCHITEW6432']` ->
   `process.env.PROCESSOR_ARCHITEW6432`.

2. `path.dirname(__filename)` -> `__dirname`.

3. `path.resolve(__dirname)` -> `__dirname`.

PR-URL: #7758
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants