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.

Sign up for GitHub

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

Jump to bottom

[v8.x] backport some assert commits #23223

Closed
wants to merge 8 commits into from
Closed

[v8.x] backport some assert commits #23223

wants to merge 8 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Oct 2, 2018

Using assert became much more friendly recently. As long as we still have the chance to get some commits into 8 I thought I go ahead and open a backport for these.

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

@BridgeAR
assert: add strict functionality export
9886376 
Requireing the strict version will allow to use `assert.equal`,
`assert.deepEqual` and there negated counterparts to be used with
strict comparison instead of using e.g. `assert.strictEqual`.

The API is identical to the regular assert export and only differs
in the way that all functions use strict compairson.

PR-URL: nodejs#17002
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BridgeAR
assert: fix .throws operator
14ee8be 
assert.throws and assert.doesNotThrow set the operator to a
internal function. That was by accident and originally the operator
was undefined. This changes it to show "throws" or "doesNotThrow".

PR-URL: nodejs#17575
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@BridgeAR
assert: use object argument in innerFail
c947fe7 
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>
@BridgeAR
assert: fix throws and doesNotThrow stack frames
a262ffa 
The stack frames from .throws and .doesNotThrow got included
even though that was not intended.

PR-URL: nodejs#17703
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
assert: improve assert.throws
ac8c99a 
Throw a TypeError in case a error message is provided in the second
argument and a third argument is present as well.
This is clearly a mistake and should not be done.

PR-URL: nodejs#17585
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
@BridgeAR
assert: .throws accept objects
f656075 
From now on it is possible to use a validation object in throws
instead of the other possibilites.

PR-URL: nodejs#17584
Refs: nodejs#17557
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
@BridgeAR
assert: fix strict regression
025d7de 
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
assert: fix throws trace
dcfda69 
The current stack trace thrown in case `assert.throws(fn, object)`
is used did not filter the stack trace. This fixes it.

PR-URL: nodejs#18595
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@nodejs-github-bot
Copy link
Collaborator

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1098/pipeline

@nodejs-github-bot 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. v8.x labels Oct 2, 2018
@BethGriggs
Copy link
Member

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

@MylesBorins
Copy link
Contributor

one more ci: https://ci.nodejs.org/job/node-test-pull-request/18259/

@MylesBorins
Copy link
Contributor

landed in e3bddee...18071db

MylesBorins pushed a commit that referenced this pull request Oct 31, 2018
@BridgeAR @MylesBorins
assert: add strict functionality export
c6d94f8 
Requireing the strict version will allow to use `assert.equal`,
`assert.deepEqual` and there negated counterparts to be used with
strict comparison instead of using e.g. `assert.strictEqual`.

The API is identical to the regular assert export and only differs
in the way that all functions use strict compairson.

Backport-PR-URL: #23223
PR-URL: #17002
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Oct 31, 2018
@BridgeAR @MylesBorins
assert: fix .throws operator
84948cf 
assert.throws and assert.doesNotThrow set the operator to a
internal function. That was by accident and originally the operator
was undefined. This changes it to show "throws" or "doesNotThrow".

Backport-PR-URL: #23223
PR-URL: #17575
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
MylesBorins pushed a commit that referenced this pull request Oct 31, 2018
@BridgeAR @MylesBorins
assert: use object argument in innerFail
a42d072 
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>
MylesBorins pushed a commit that referenced this pull request Oct 31, 2018
@BridgeAR @MylesBorins
assert: fix throws and doesNotThrow stack frames
c9d84b6 
The stack frames from .throws and .doesNotThrow got included
even though that was not intended.

Backport-PR-URL: #23223
PR-URL: #17703
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 31, 2018
@BridgeAR @MylesBorins
assert: improve assert.throws
147aeed 
Throw a TypeError in case a error message is provided in the second
argument and a third argument is present as well.
This is clearly a mistake and should not be done.

Backport-PR-URL: #23223
PR-URL: #17585
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
MylesBorins pushed a commit that referenced this pull request Oct 31, 2018
@BridgeAR @MylesBorins
assert: .throws accept objects
f2af930 
From now on it is possible to use a validation object in throws
instead of the other possibilites.

Backport-PR-URL: #23223
PR-URL: #17584
Refs: #17557
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
MylesBorins pushed a commit that referenced this pull request Oct 31, 2018
@BridgeAR @MylesBorins
assert: fix strict regression
562787e 
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>
MylesBorins pushed a commit that referenced this pull request Oct 31, 2018
@BridgeAR @MylesBorins
assert: fix throws trace
18071db 
The current stack trace thrown in case `assert.throws(fn, object)`
is used did not filter the stack trace. This fixes it.

Backport-PR-URL: #23223
PR-URL: #18595
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@MylesBorins MylesBorins added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 31, 2018
@BethGriggs BethGriggs mentioned this pull request Nov 20, 2018
Merged
BethGriggs added a commit that referenced this pull request Nov 20, 2018
@BethGriggs
2018-11-20, Version 8.13.0 'Carbon' (LTS)
20e68dd 
Notable changes:

* **assert**:
  - backport some assert commits (Ruben Bridgewater) [#23223](#23223)
* **deps**:
  - V8: cherry-pick 64-bit hash seed commits (Yang Guo) [#23274](#23274)
* **http**:
  - added aborted property to request (Robert Nagy) [#20094](#20094)
* **http2**:
  - backport http2 changes from 10.x (Kelvin Jin) [#22850](#22850)

PR-URL: #23974
MylesBorins pushed a commit that referenced this pull request Nov 20, 2018
@BethGriggs @MylesBorins
2018-11-20, Version 8.13.0 'Carbon' (LTS)
90c7bfa 
Notable changes:

* **assert**:
  - backport some assert commits (Ruben Bridgewater)
    [#23223](#23223)
* **deps**:
  - upgrade to libuv 1.23.2 (cjihrig)
    [#23336](#23336)
  - V8: cherry-pick 64-bit hash seed commits (Yang Guo)
    [#23274](#23274)
* **http**:
  - added aborted property to request (Robert Nagy)
    [#20094](#20094)
* **http2**:
  - graduate from experimental (James M Snell)
    [#22466](#22466)

PR-URL: #23974
MylesBorins pushed a commit that referenced this pull request Nov 20, 2018
@BethGriggs @MylesBorins
2018-11-20, Version 8.13.0 'Carbon' (LTS)
2c2e6ec 
Notable changes:

* **assert**:
  - backport some assert commits (Ruben Bridgewater)
    [#23223](#23223)
* **deps**:
  - upgrade to libuv 1.23.2 (cjihrig)
    [#23336](#23336)
  - V8: cherry-pick 64-bit hash seed commits (Yang Guo)
    [#23274](#23274)
* **http**:
  - added aborted property to request (Robert Nagy)
    [#20094](#20094)
* **http2**:
  - graduate from experimental (James M Snell)
    [#22466](#22466)

PR-URL: #23974
@beevelop beevelop mentioned this pull request Nov 20, 2018
Closed
@SimenB SimenB mentioned this pull request Dec 2, 2018
Merged
@SimenB
Copy link
Member

SimenB commented Dec 2, 2018 •
edited
Loading

The backport of #17585 (147aeed) included #12167 (adding actual.message in the error for notThrows), which broke one of Jest's test for Node 8.

It's definitely an improvement, but it would be nice if it had been part of the changelog for 8.13 so it would have been easier to find 🙂

(PR fixing Jest's test suite: jestjs/jest#7446)

@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 2, 2018

It was not intentional to backport that part as it's semver-major. I know I tried to keep it out while rebasing but it seemed like it still sneaked in. I am going to open a fix for that.

@SimenB
Copy link
Member

SimenB commented Dec 2, 2018 •
edited
Loading

I've fixed Jest's test suite (it was simply a matter of treating node 8 same as 9 in the test we have with node core assert, which IMO is an improvement), so I'm not advocating for a fix, just pointing it out.
But if you think it's breaking I can just revert the change once a patch is out

BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 2, 2018
@BridgeAR
assert: revert breaking change
bd7004f 
It was not intended to change the `assert.doesNotThrow()` message
with nodejs#23223. This reverts the
breaking behavior to the one before.
@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 2, 2018

I agree that it is an improvement but it was never meant to be released on 8.x.

I guess there will be little harm keeping it but I am still opening a revert and keep the decision to other collaborators.

@BridgeAR BridgeAR mentioned this pull request Dec 2, 2018
Closed
4 tasks
BethGriggs pushed a commit that referenced this pull request Dec 11, 2018
@BridgeAR @BethGriggs
assert: revert breaking change
62fb5db 
It was not intended to change the `assert.doesNotThrow()` message
with #23223. This reverts the
breaking behavior to the one before.

PR-URL: #24786
Refs: #23223
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@BethGriggs @refack
2018-11-20, Version 8.13.0 'Carbon' (LTS)
7b4a62b 
Notable changes:

* **assert**:
  - backport some assert commits (Ruben Bridgewater)
    [nodejs#23223](nodejs#23223)
* **deps**:
  - upgrade to libuv 1.23.2 (cjihrig)
    [nodejs#23336](nodejs#23336)
  - V8: cherry-pick 64-bit hash seed commits (Yang Guo)
    [nodejs#23274](nodejs#23274)
* **http**:
  - added aborted property to request (Robert Nagy)
    [nodejs#20094](nodejs#20094)
* **http2**:
  - graduate from experimental (James M Snell)
    [nodejs#22466](nodejs#22466)

PR-URL: nodejs#23974
@Fleta Fleta mentioned this pull request Jan 24, 2019
Merged
@BridgeAR BridgeAR deleted the backport-assert branch January 20, 2020 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
assert Issues and PRs related to the assert subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@BridgeAR @nodejs-github-bot @BethGriggs @MylesBorins @SimenB