Skip to content

test: refactor common.js #9732

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 1 commit into from
Closed

test: refactor common.js #9732

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Nov 22, 2016

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

test

Description of change
  • remove unused common.faketimeCli
  • remove mosly-unused common.testDir
  • assert.ok(false...) -> fail()
  • alphabetize list of known globals
  • .indexOf() -> .includes()

/cc @nodejs/testing

* remove unused common.faketimeCli
* remove mosly-unused common.testDir
* assert.ok(false...) -> fail()
* alphabetize list of known globals
* .indexOf() -> .includes()
@Trott Trott added the test Issues and PRs related to the tests. label Nov 22, 2016
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 22, 2016
Copy link
Member

@santigimeno santigimeno left a comment

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

@santigimeno
Copy link
Member

Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

LGTM w/ CI

@mhdawson
Copy link
Member

The CI ran hung on AIX so I'm starting a new one to see if it was related:

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

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM provided second CI run ok

@Trott
Copy link
Member Author

Trott commented Nov 24, 2016

Test failure in CI is an unrelated flaky that will hopefully be fixed by #9791 but just to be extra-cautious with changes to common.js:

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

Trott added a commit to Trott/io.js that referenced this pull request Nov 26, 2016
* remove unused common.faketimeCli
* remove mosly-unused common.testDir
* assert.ok(false...) -> fail()
* alphabetize list of known globals
* .indexOf() -> .includes()

PR-URL: nodejs#9732
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@Trott
Copy link
Member Author

Trott commented Nov 26, 2016

Landed in 561eade

@Trott Trott closed this Nov 26, 2016
addaleax pushed a commit that referenced this pull request Dec 5, 2016
* remove unused common.faketimeCli
* remove mosly-unused common.testDir
* assert.ok(false...) -> fail()
* alphabetize list of known globals
* .indexOf() -> .includes()

PR-URL: #9732
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@Fishrock123 Fishrock123 mentioned this pull request Dec 5, 2016
2 tasks
addaleax pushed a commit to addaleax/node that referenced this pull request Dec 8, 2016
* remove unused common.faketimeCli
* remove mosly-unused common.testDir
* assert.ok(false...) -> fail()
* alphabetize list of known globals
* .indexOf() -> .includes()

PR-URL: nodejs#9732
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
* remove unused common.faketimeCli
* remove mosly-unused common.testDir
* assert.ok(false...) -> fail()
* alphabetize list of known globals
* .indexOf() -> .includes()

PR-URL: #9732
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 21, 2016
@Trott Trott deleted the globals branch January 13, 2022 22:44
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants