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: s/assert.fail/common.fail as appropriate #7735

Merged
merged 1 commit into from
Jul 15, 2016
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jul 14, 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

Many tests use assert.fail(null, null, msg) where it would be simpler to use common.fail(msg). This is largely because common.fail() is fairly new. This commit makes the replacement when applicable.

R= @nodejs/testing

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 14, 2016
@thefourtheye
Copy link
Contributor

LGTM, if CI is green.

@@ -23,7 +23,7 @@ server.listen(0, () => {
// Send two content-length header values.
headers: {'Content-Length': [1, 2]}},
(res) => {
assert.fail(null, null, 'an error should have occurred');
common.fail('an error should have occurred');
server.close();
Copy link
Member

Choose a reason for hiding this comment

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

Dead code.

@bnoordhuis
Copy link
Member

LGTM

@cjihrig cjihrig force-pushed the fail branch 2 times, most recently from 1801500 to 1a14f45 Compare July 15, 2016 14:42
@cjihrig
Copy link
Contributor Author

cjihrig commented Jul 15, 2016

@Fishrock123
Copy link
Contributor

LGTM if CI is green

@cjihrig
Copy link
Contributor Author

cjihrig commented Jul 15, 2016

CI was green, minus one buildbot that appears to have gone offline (which is consistent with the last few CI runs). Trying again. CI: https://ci.nodejs.org/job/node-test-pull-request/3301/

@cjihrig
Copy link
Contributor Author

cjihrig commented Jul 15, 2016

Still with the ppcbe-ubuntu1404 buildbot. Taking it as a green CI.

Many tests use assert.fail(null, null, msg) where it would be
simpler to use common.fail(msg). This is largely because
common.fail() is fairly new. This commit makes the replacement
when applicable.

PR-URL: nodejs#7735
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@cjihrig cjihrig merged commit 6510eb5 into nodejs:master Jul 15, 2016
@cjihrig cjihrig deleted the fail branch July 15, 2016 19:55
evanlucas pushed a commit that referenced this pull request Jul 19, 2016
Many tests use assert.fail(null, null, msg) where it would be
simpler to use common.fail(msg). This is largely because
common.fail() is fairly new. This commit makes the replacement
when applicable.

PR-URL: #7735
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
evanlucas pushed a commit that referenced this pull request Jul 20, 2016
Many tests use assert.fail(null, null, msg) where it would be
simpler to use common.fail(msg). This is largely because
common.fail() is fairly new. This commit makes the replacement
when applicable.

PR-URL: #7735
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 30, 2016
Many tests use assert.fail(null, null, msg) where it would be
simpler to use common.fail(msg). This is largely because
common.fail() is fairly new. This commit makes the replacement
when applicable.

PR-URL: #7735
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
Many tests use assert.fail(null, null, msg) where it would be
simpler to use common.fail(msg). This is largely because
common.fail() is fairly new. This commit makes the replacement
when applicable.

PR-URL: #7735
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
Many tests use assert.fail(null, null, msg) where it would be
simpler to use common.fail(msg). This is largely because
common.fail() is fairly new. This commit makes the replacement
when applicable.

PR-URL: #7735
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.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.

6 participants