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

assert: minor assert improvements #27525

Closed
wants to merge 4 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented May 1, 2019

See commit messages for further details.

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

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label May 1, 2019
@BridgeAR BridgeAR requested a review from Trott May 2, 2019 20:46
@nodejs-github-bot

This comment has been minimized.

@BridgeAR BridgeAR requested a review from richardlau May 2, 2019 20:47
This makes sure the error message visible in the error stack created
when using `assert.fail()` without any arguments or the message set
to `undefined` or `null` as only argument.
That was masked before due to other changes.
This makes sure that the error message is more appropriate than
before by checking closer what operator is used and which is not.

It also increases the total number of lines printed to the user.
This reduces the total amount of reads when using `assert.ok()` with
a falsy value. That increases the read performance significantly.

Also remove a comment that can not be addressed.
@BridgeAR BridgeAR force-pushed the minor-assert-improvements branch from aca30b0 to d993bb9 Compare May 2, 2019 21:32
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot

This comment has been minimized.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 3, 2019
lib/assert.js Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented May 5, 2019

Landed in 7cfcf80...d099f2f

@Trott Trott closed this May 5, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request May 5, 2019
This makes sure the error message visible in the error stack created
when using `assert.fail()` without any arguments or the message set
to `undefined` or `null` as only argument.
That was masked before due to other changes.

PR-URL: nodejs#27525
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Trott pushed a commit to Trott/io.js that referenced this pull request May 5, 2019
This makes sure that the error message is more appropriate than
before by checking closer what operator is used and which is not.

It also increases the total number of lines printed to the user.

PR-URL: nodejs#27525
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Trott pushed a commit to Trott/io.js that referenced this pull request May 5, 2019
This reduces the total amount of reads when using `assert.ok()` with
a falsy value. That increases the read performance significantly.

Also remove a comment that can not be addressed.

PR-URL: nodejs#27525
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@targos
Copy link
Member

targos commented May 6, 2019

This needs to be backported to land on v12.x:

$ out/Release/node --expose-internals test/parallel/test-assert.js
assert.js:540
      throw err;
      ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected

  Comparison {
    generatedMessage: true,
+   message: "Expected values to be strictly equal:\n\n[Function] !== ''\n"
-   message: "Expected values to be strictly equal:\n+ actual - expected\n\n+ [Function]\n- ''"
  }
    at testAssertionMessage (/home/mzasso/git/nodejs/v12.x/test/parallel/test-assert.js:267:10)
    at Object.<anonymous> (/home/mzasso/git/nodejs/v12.x/test/parallel/test-assert.js:300:1)
    at Module._compile (internal/modules/cjs/loader.js:759:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:770:10)
    at Module.load (internal/modules/cjs/loader.js:628:32)
    at Function.Module._load (internal/modules/cjs/loader.js:555:12)
    at Function.Module.runMain (internal/modules/cjs/loader.js:822:10)
    at internal/main/run_main_module.js:17:11

BridgeAR added a commit that referenced this pull request May 6, 2019
This makes sure the error message visible in the error stack created
when using `assert.fail()` without any arguments or the message set
to `undefined` or `null` as only argument.
That was masked before due to other changes.

PR-URL: #27525
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
BridgeAR added a commit that referenced this pull request May 6, 2019
This makes sure that the error message is more appropriate than
before by checking closer what operator is used and which is not.

It also increases the total number of lines printed to the user.

PR-URL: #27525
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
BridgeAR added a commit that referenced this pull request May 6, 2019
This reduces the total amount of reads when using `assert.ok()` with
a falsy value. That increases the read performance significantly.

Also remove a comment that can not be addressed.

PR-URL: #27525
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@BridgeAR
Copy link
Member Author

BridgeAR commented May 6, 2019

@targos I pushed the backport directly onto the staging branch. It was just a very tiny conflict.

--- a/test/parallel/test-assert.js
+++ b/test/parallel/test-assert.js
@@ -291,13 +291,13 @@ testShortAssertionMessage(0, '0');
 testShortAssertionMessage(Symbol(), 'Symbol()');
 testShortAssertionMessage(undefined, 'undefined');
 testShortAssertionMessage(-Infinity, '-Infinity');
+testShortAssertionMessage(function() {}, '[Function]');
 testAssertionMessage([], '[]');
 testAssertionMessage(/a/, '/a/');
 testAssertionMessage(/abc/gim, '/abc/gim');
 testAssertionMessage({}, '{}');
 testAssertionMessage([1, 2, 3], '[\n+   1,\n+   2,\n+   3\n+ ]');
 testAssertionMessage(function f() {}, '[Function: f]');
-testAssertionMessage(function() {}, '[Function]');
 testAssertionMessage(circular, '{\n+   x: [Circular],\n+   y: 1\n+ }');
 testAssertionMessage({ a: undefined, b: null },
                      '{\n+   a: undefined,\n+   b: null\n+ }');

targos pushed a commit that referenced this pull request May 7, 2019
This makes sure the error message visible in the error stack created
when using `assert.fail()` without any arguments or the message set
to `undefined` or `null` as only argument.
That was masked before due to other changes.

PR-URL: #27525
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this pull request May 7, 2019
This makes sure that the error message is more appropriate than
before by checking closer what operator is used and which is not.

It also increases the total number of lines printed to the user.

PR-URL: #27525
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this pull request May 7, 2019
This reduces the total amount of reads when using `assert.ok()` with
a falsy value. That increases the read performance significantly.

Also remove a comment that can not be addressed.

PR-URL: #27525
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@BridgeAR BridgeAR mentioned this pull request May 21, 2019
4 tasks
@BridgeAR BridgeAR deleted the minor-assert-improvements branch January 20, 2020 12:03
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. author ready PRs that have at least one approval, no pending requests for changes, and a CI started.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants