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

[v8.x] backport some assert commits #23223

Closed
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

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>
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>
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>
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>
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>
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>
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>
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 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

@MylesBorins
Copy link
Contributor

@MylesBorins
Copy link
Contributor

landed in e3bddee...18071db

MylesBorins pushed a commit that referenced this pull request Oct 31, 2018
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
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
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
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
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
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
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
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
BethGriggs added a commit that referenced this pull request Nov 20, 2018
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
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
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
@SimenB
Copy link
Member

SimenB commented Dec 2, 2018

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

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
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.

BethGriggs pushed a commit that referenced this pull request Dec 11, 2018
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
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
@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
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
Development

Successfully merging this pull request may close these issues.

5 participants