assert: use object argument in innerFail #17582

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 1cd7c4d Compare 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assert: use object argument in innerFail #17582

assert: use object argument in innerFail #17582

BridgeAR commented Dec 9, 2017 •

edited

addaleax commented Dec 9, 2017

BridgeAR commented Dec 9, 2017

BridgeAR commented Dec 12, 2017

apapirovski commented Dec 14, 2017

BridgeAR commented Dec 15, 2017

BridgeAR commented Dec 15, 2017

MylesBorins commented Jan 9, 2018 •

edited

BridgeAR commented Mar 8, 2018

MylesBorins commented Jul 31, 2018 •

edited

LinusU commented Aug 8, 2018

assert: use object argument in innerFail #17582

assert: use object argument in innerFail #17582

Conversation

BridgeAR commented Dec 9, 2017 • edited

Checklist

Affected core subsystem(s)

addaleax commented Dec 9, 2017

BridgeAR commented Dec 9, 2017

BridgeAR commented Dec 12, 2017

apapirovski commented Dec 14, 2017

BridgeAR commented Dec 15, 2017

BridgeAR commented Dec 15, 2017

MylesBorins commented Jan 9, 2018 • edited

BridgeAR commented Mar 8, 2018

MylesBorins commented Jul 31, 2018 • edited

LinusU commented Aug 8, 2018

BridgeAR commented Dec 9, 2017 •

edited

MylesBorins commented Jan 9, 2018 •

edited

MylesBorins commented Jul 31, 2018 •

edited