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: refactor AssertionError properties #25250

Closed
wants to merge 5 commits into from

Conversation

@BridgeAR
Copy link
Member

commented Dec 28, 2018

When inspecting an AssertionError the actual and expected properties would also show up inspected (due to another recent change that made sure that extra properties on errors will always be visible).

This is not really very user friendly since we already print something in the error message and showing the values again, duplicates the output. Another alternative to this would be to use a custom inspection function for AssertionError but I believe just moving these two properties behind getters already solved the problem well enough while keeping the other properties visible to the user.

The other commit fixes a test case.

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 added 2 commits Dec 28, 2018
assert: make `actual` and `expected` getters
The `actual` and `expected` properties on an instance of
`AssertionError` is now a getter to prevent inspecting these when
inspecting the error. These values will be visible in the error
message and showing them otherwise would decrease the readability
of the error.
test: fix failing assertion
One test did not cause an assertion. By changing the test to use
`assert.throws()` all tests have to throw, otherwise the test will
fail.
@Trott
Copy link
Member

left a comment

Test change/fix LGTM. No comment on the behavior change of not inspecting expected/actual. I'd need to think more on the ramifications there. (Maybe a before/after comparison to show the difference would be illuminating.)

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Dec 28, 2018

Before:

> assert.deepStrictEqual(Array(25).fill(10), 2)
{ [AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected ... Lines skipped

+ [
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
...
- 2
...]
  generatedMessage: true,
  name: 'AssertionError [ERR_ASSERTION]',
  code: 'ERR_ASSERTION',
  actual:
   [ 10,
     10,
     10,
     10,
     10,
     10,
     10,
     10,
     10,
     10,
     10,
     10,
     10,
     10,
     10,
     10,
     10,
     10,
     10,
     10,
     10,
     10,
     10,
     10,
     10 ],
  expected: 2,
  operator: 'deepStrictEqual' }

After

assert.deepStrictEqual(Array(25).fill(10), 2)
{ [AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected ... Lines skipped

+ [
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
...
- 2
...]
  generatedMessage: true,
  name: 'AssertionError [ERR_ASSERTION]',
  code: 'ERR_ASSERTION',
  actual: [Getter/Setter],
  expected: [Getter/Setter],
  operator: 'deepStrictEqual' }
@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2018

  actual: [Getter/Setter],
  expected: [Getter/Setter],

That output might be shorter, but I don't think it's helpful at all.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Dec 28, 2018

@cjihrig

That output might be shorter, but I don't think it's helpful at all.

That output itself is only meant so the user known it's possible to access those properties. As I outlined above, these entries were not inspected at all until recently. Now that the extra properties are inspected by default we'd duplicate information and hide the actual error message above other information. That's why I choose this as a good middle ground.

Do you have a better alternative? (It's still possible to use a custom inspection function as well)

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2018

I can imagine the Getter/Setter output being especially misleading to newcomers. A custom inspection function that restores the old behavior should be fine.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Dec 28, 2018

@cjihrig yeah, that output is indeed subpar. I believe I found another good way to display this:

assert.deepStrictEqual(Array(25).fill(10), 2)
{ [AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal:
+ actual - expected ... Lines skipped

+ [
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
+   10,
...
- 2
...]
  generatedMessage: true,
  name: 'AssertionError [ERR_ASSERTION]',
  code: 'ERR_ASSERTION',
  actual: [Array],
  expected: 2,
  operator: 'deepStrictEqual' }
BridgeAR added 2 commits Dec 28, 2018
lib/internal/assert.js Outdated Show resolved Hide resolved
@addaleax

This comment has been minimized.

Copy link
Member

commented Jan 7, 2019

@jasnell
jasnell approved these changes Jan 8, 2019
fixup: fix typo
Co-Authored-By: BridgeAR <ruben@bridgewater.de>
@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

I just started the lite CI because there's only a typo fix and the last full CI is green.

Lite-CI https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/2193/
Lite-CI https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/2201/

@BridgeAR

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2019

Landed in dfaa61f, 8c0290e 🎉

Thanks everyone for the reviews.

@BridgeAR BridgeAR closed this Jan 10, 2019

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 10, 2019
assert: make `actual` and `expected` getters
The `actual` and `expected` properties on an instance of
`AssertionError` is now a getter to prevent inspecting these when
inspecting the error. These values will be visible in the error
message and showing them otherwise would decrease the readability
of the error.

PR-URL: nodejs#25250
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 10, 2019
test: fix failing assertion
One test did not cause an assertion. By changing the test to use
`assert.throws()` all tests have to throw, otherwise the test will
fail.

PR-URL: nodejs#25250
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Jan 14, 2019
assert: make `actual` and `expected` getters
The `actual` and `expected` properties on an instance of
`AssertionError` is now a getter to prevent inspecting these when
inspecting the error. These values will be visible in the error
message and showing them otherwise would decrease the readability
of the error.

PR-URL: #25250
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Jan 14, 2019
test: fix failing assertion
One test did not cause an assertion. By changing the test to use
`assert.throws()` all tests have to throw, otherwise the test will
fail.

PR-URL: #25250
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR referenced this pull request Jan 16, 2019
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
assert: make `actual` and `expected` getters
The `actual` and `expected` properties on an instance of
`AssertionError` is now a getter to prevent inspecting these when
inspecting the error. These values will be visible in the error
message and showing them otherwise would decrease the readability
of the error.

PR-URL: nodejs#25250
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
test: fix failing assertion
One test did not cause an assertion. By changing the test to use
`assert.throws()` all tests have to throw, otherwise the test will
fail.

PR-URL: nodejs#25250
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins referenced this pull request Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.