assert: use object argument in innerFail by BridgeAR · Pull Request #17582 · nodejs/node

BridgeAR · 2017-12-09T22:35:47Z

Right now it is difficult to know what argument stands for what
property. By refactoring the arguments into a object it is clear
what it means (It happened before that they got mixed up. See #17575).

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

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

assert

addaleax · 2017-12-09T22:46:30Z

not ok 351 parallel/test-dgram-create-socket-handle
---
duration_ms: 0.65
severity: fail
stack: |-
assert.js:42
throw new errors.AssertionError(obj);
^
AssertionError [ERR_ASSERTION]: undefined == true does not match /^false == true$/
at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/common/index.js:744:9)
at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/common/index.js:522:15)
at expectedException (assert.js:208:19)
at innerThrows (assert.js:247:21)
at Function.throws (assert.js:266:3)
at Object.expectsError (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/common/index.js:764:12)
at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-dgram-create-socket-handle.js:9:8)
at Module._compile (module.js:660:30)
at Object.Module._extensions..js (module.js:671:10)
at Module.load (module.js:577:32)

Seems like it might be related?

BridgeAR · 2017-12-09T22:59:26Z

@addaleax jupp, I only ran the assert tests. I now added a specific test for that to catch something not in a completely independent one. Thanks for pointing it out.

BridgeAR · 2017-12-12T21:15:47Z

CI https://ci.nodejs.org/job/node-test-commit-light/19/

apapirovski · 2017-12-14T18:38:56Z

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

Some of the Windows tests didn't run last time. Just being extra safe.

Right now it is difficult to know what argument stands for what property. By refactoring the arguments into a object it is clear what stands for what.

BridgeAR · 2017-12-15T21:25:33Z

New CI due to a rebase https://ci.nodejs.org/job/node-test-commit-light/28/

BridgeAR · 2017-12-15T22:43:13Z

Landed in 7cf569a

Right now it is difficult to know what argument stands for what property. By refactoring the arguments into a object it is clear what stands for what. PR-URL: nodejs#17582 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

Using `assert()` or `assert.ok()` resulted in a error since a refactoring. Refs: nodejs#17582

Using `assert()` or `assert.ok()` resulted in a error since a refactoring. Refs: nodejs#17582 PR-URL: nodejs#17903 Refs: nodejs#17582 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2018-01-09T22:41:52Z

This does not land cleanly on v9.x, could it please be backported

edit: it should come with #17903

Right now it is difficult to know what argument stands for what property. By refactoring the arguments into a object it is clear what stands for what. PR-URL: nodejs#17582 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

Using `assert()` or `assert.ok()` resulted in a error since a refactoring. Refs: nodejs#17582 PR-URL: nodejs#17903 Refs: nodejs#17582 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR · 2018-03-08T16:23:08Z

Backport opened in #19230

Right now it is difficult to know what argument stands for what property. By refactoring the arguments into a object it is clear what stands for what. Backport-PR-URL: #19230 PR-URL: #17582 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

Using `assert()` or `assert.ok()` resulted in a error since a refactoring. Refs: #17582 Backport-PR-URL: #19230 PR-URL: #17903 Refs: #17582 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Right now it is difficult to know what argument stands for what property. By refactoring the arguments into a object it is clear what stands for what. Backport-PR-URL: #19230 PR-URL: #17582 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

Using `assert()` or `assert.ok()` resulted in a error since a refactoring. Refs: #17582 Backport-PR-URL: #19230 PR-URL: #17903 Refs: #17582 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2018-07-31T18:24:59Z

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

Requested backport to 8.x in #19230

LinusU · 2018-08-08T15:20:12Z

I'm very late to the party here but just noticed that this is a breaking change for a package I'm maintaining. Fortunately, in this specific case, I haven't published the version that will break in Node.js 10 yet. But there are potentially other usages of this.

This PR changes the name of stackStartFunction to stackStartFn, even the key in the options argument of the publicly visible AssertionError.

My current workaround is to pass both stackStartFn and stackStartFunction, but I think it was a bit unfortunate that this change landed...

The package is assert-rejects btw. https://github.com/LinusU/assert-rejects

I'm trying to change assert.fail(false, error, 'Missing expected rejection' + message) into using throw AssertionError directly, since fail with three arguments have been deprecated.

Right now it is difficult to know what argument stands for what property. By refactoring the arguments into a object it is clear what stands for what. PR-URL: nodejs#17582 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

Using `assert()` or `assert.ok()` resulted in a error since a refactoring. Refs: nodejs#17582 PR-URL: nodejs#17903 Refs: nodejs#17582 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Right now it is difficult to know what argument stands for what property. By refactoring the arguments into a object it is clear what stands for what. Backport-PR-URL: #23223 PR-URL: #17582 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

Using `assert()` or `assert.ok()` resulted in a error since a refactoring. Refs: #17582 Backport-PR-URL: #23223 PR-URL: #17903 Refs: #17582 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added assert Issues and PRs related to the assert subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Dec 9, 2017

addaleax approved these changes Dec 9, 2017
View reviewed changes

maclover7 approved these changes Dec 10, 2017
View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 12, 2017

jasnell approved these changes Dec 12, 2017
View reviewed changes

assert: use object argument in innerFail
1cd7c4d
Right now it is difficult to know what argument stands for what property. By refactoring the arguments into a object it is clear what stands for what.

BridgeAR force-pushed the improve-innerFail branch from 5da67eb to 1cd7c4dCompare December 15, 2017 21:24

apapirovski approved these changes Dec 15, 2017
View reviewed changes

BridgeAR closed this Dec 15, 2017

BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 28, 2017
assert: fix strict regression
64571fa
Using `assert()` or `assert.ok()` resulted in a error since a refactoring. Refs: nodejs#17582

BridgeAR mentioned this pull request Dec 28, 2017
assert: fix strict regression #17903
Closed
4 tasks

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 29, 2017

MylesBorins added the backport-requested-v9.x label Jan 9, 2018

BridgeAR mentioned this pull request Mar 8, 2018
[9.x backport] assert & util PRs #19230
Closed
4 tasks

targos mentioned this pull request Mar 18, 2018
v9.9.0 proposal #19428
Merged

targos added backported-to-v9.x and removed backport-requested-v9.x labels Mar 24, 2018

MylesBorins added the backport-requested-v8.x label Jul 31, 2018

MylesBorins added backported-to-v8.x and removed backport-requested-v8.x labels Oct 31, 2018

BethGriggs mentioned this pull request Nov 12, 2018
v8.13.0 proposal #23974
Merged

beevelop mentioned this pull request Nov 20, 2018
2018-11-20 Version 8.13.0 'Carbon' (LTS) @MylesBorins prepared by @BethGriggs beevelop/docker-android-nodejs#250
Closed

BridgeAR deleted the improve-innerFail branch April 1, 2019 23:37

Navigation Menu

Uh oh!

Uh oh!

assert: use object argument in innerFail - #17582

assert: use object argument in innerFail#17582
BridgeAR wants to merge 1 commit into
nodejs:masterfrom
BridgeAR:improve-innerFail

BridgeAR commented Dec 9, 2017•
edited
Loading

Uh oh!

addaleax commented Dec 9, 2017

Uh oh!

BridgeAR commented Dec 9, 2017

Uh oh!

BridgeAR commented Dec 12, 2017

Uh oh!

apapirovski commented Dec 14, 2017

Uh oh!

BridgeAR commented Dec 15, 2017

Uh oh!

BridgeAR commented Dec 15, 2017

Uh oh!

MylesBorins commented Jan 9, 2018•
edited
Loading

Uh oh!

BridgeAR commented Mar 8, 2018

Uh oh!

MylesBorins commented Jul 31, 2018•
edited
Loading

Uh oh!

LinusU commented Aug 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Uh oh!

Uh oh!

Conversation

BridgeAR commented Dec 9, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

addaleax commented Dec 9, 2017

Uh oh!

BridgeAR commented Dec 9, 2017

Uh oh!

BridgeAR commented Dec 12, 2017

Uh oh!

apapirovski commented Dec 14, 2017

Uh oh!

BridgeAR commented Dec 15, 2017

Uh oh!

BridgeAR commented Dec 15, 2017

Uh oh!

MylesBorins commented Jan 9, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Mar 8, 2018

Uh oh!

MylesBorins commented Jul 31, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LinusU commented Aug 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

BridgeAR commented Dec 9, 2017•
edited
Loading

MylesBorins commented Jan 9, 2018•
edited
Loading

MylesBorins commented Jul 31, 2018•
edited
Loading