Skip to content

process: fix incorrect usage of assert.fail() - #6211

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:deep
Closed

process: fix incorrect usage of assert.fail()#6211
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:deep

Conversation

@Trott

@TrottTrott commented Apr 15, 2016

Copy link
Copy Markdown
Member
Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

process, promises

Description of change

The message argument for assert.fail() is the third argument, not the
first. Correct minor misuse in internal module.

@Trott
process: fix incorrect usage of assert.fail()
c60a3f0
The message argument for `assert.fail()` is the third argument, not the
first. Correct minor misuse in internal module.
@TrottTrott added process Issues and PRs related to the process subsystem. promises Issues and PRs related to ECMAScript promises. labels Apr 15, 2016
@Trott

Trott commented Apr 15, 2016

Copy link
Copy Markdown
MemberAuthor

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

mscdex
mscdex reviewed Apr 15, 2016
View reviewed changes
Comment threadlib/internal/process/promises.js
rejectionHandled(promise);
else
require('assert').fail('unexpected PromiseRejectEvent');
require('assert').fail(null, null, 'unexpected PromiseRejectEvent');

@mscdexmscdexApr 15, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is the require() here instead of at the top of the file?

@bnoordhuisbnoordhuisApr 15, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

To avoid the hit of require('assert') in the common case.

@mscdex

mscdex commented Apr 15, 2016

Copy link
Copy Markdown
Contributor

1 nit, but otherwise LGTM

@bnoordhuis

bnoordhuis commented Apr 15, 2016

Copy link
Copy Markdown
Member

LGTM

3 similar comments
@jasnell

jasnell commented Apr 15, 2016

Copy link
Copy Markdown
Member

LGTM

@evanlucas

evanlucas commented Apr 18, 2016

Copy link
Copy Markdown
Contributor

LGTM

@cjihrig

cjihrig commented Apr 18, 2016

Copy link
Copy Markdown
Contributor

LGTM

jasnell pushed a commit that referenced this pull request Apr 18, 2016
@Trott@jasnell
process: fix incorrect usage of assert.fail()
b7f4b1b
The message argument for `assert.fail()` is the third argument, not the
first. Correct minor misuse in internal module.
PR-URL: #6211
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell

jasnell commented Apr 18, 2016

Copy link
Copy Markdown
Member

Landed in b7f4b1b

@jasnelljasnell closed this Apr 18, 2016
MylesBorins pushed a commit that referenced this pull request Apr 19, 2016
@TrottMyles Borins
process: fix incorrect usage of assert.fail()
8af53a8
The message argument for `assert.fail()` is the third argument, not the
first. Correct minor misuse in internal module.
PR-URL: #6211
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
@TrottMyles Borins
process: fix incorrect usage of assert.fail()
4503098
The message argument for `assert.fail()` is the third argument, not the
first. Correct minor misuse in internal module.
PR-URL: #6211
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
@TrottMyles Borins
process: fix incorrect usage of assert.fail()
a31610e
The message argument for `assert.fail()` is the third argument, not the
first. Correct minor misuse in internal module.
PR-URL: #6211
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Apr 20, 2016
Closed
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
@TrottMyles Borins
process: fix incorrect usage of assert.fail()
1d40809
The message argument for `assert.fail()` is the third argument, not the
first. Correct minor misuse in internal module.
PR-URL: #6211
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This was referenced Apr 21, 2016
Closed
Closed
MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
@Trott@MylesBorins
process: fix incorrect usage of assert.fail()
fbb0a06
The message argument for `assert.fail()` is the third argument, not the
first. Correct minor misuse in internal module.
PR-URL: #6211
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
@Trott@joelostrowski
process: fix incorrect usage of assert.fail()
6517d27
The message argument for `assert.fail()` is the third argument, not the
first. Correct minor misuse in internal module.
PR-URL: nodejs#6211
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
@Trott@jasnell
process: fix incorrect usage of assert.fail()
7fd98af
The message argument for `assert.fail()` is the third argument, not the
first. Correct minor misuse in internal module.
PR-URL: #6211
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins

MylesBorins commented May 17, 2016

Copy link
Copy Markdown
Contributor

this file does not exist on v4.x

@Trott up to you if you want to figure out where the code lives

@Trott

Trott commented May 18, 2016

Copy link
Copy Markdown
MemberAuthor

@thealphanerd I think v4.x will somehow manage to get by without this patch.

@Trott
Trott deleted the deep branch January 13, 2022 22:43
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

processIssues and PRs related to the process subsystem.promisesIssues and PRs related to ECMAScript promises.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@Trott@mscdex@bnoordhuis@jasnell@evanlucas@cjihrig@MylesBorins