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: add default operator to assert.fail() #22694

Closed
wants to merge 2 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Sep 4, 2018

This makes sure assert.fail() contains an operator instead of being
undefined.

On top of that it also fixes the err.generatedMessage property.
Before, it was not always set correct.

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

This makes sure `assert.fail()` contains an operator instead of being
undefined.

On top of that it also fixes the `err.generatedMessage` property.
Before, it was not always set correct.
@nodejs-github-bot nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Sep 4, 2018
@Trott
Copy link
Member

Trott commented Sep 5, 2018

semver-major in case someone is adding a property called generatedMessage to their assertion errors? Or is that too far-fetched?

@Trott
Copy link
Member

Trott commented Sep 5, 2018

Is there a reason to expose generatedMessage? If there's no reason, to expose it then should we hide it? If there is a reason to expose it, should we document it?

@BridgeAR
Copy link
Member Author

BridgeAR commented Sep 5, 2018

@Trott the generatedMessage property was also present before. It was just not set correct all the time. It is also already documented on the AssertionError class.

@BridgeAR
Copy link
Member Author

BridgeAR commented Sep 8, 2018

PTAL @nodejs/util

@BridgeAR
Copy link
Member Author

BridgeAR commented Sep 8, 2018

@nodejs/testing

@Trott
Copy link
Member

Trott commented Sep 8, 2018

I see that operator can be 'throws' and 'doesNotThrow'. Is that a relatively recent addition? I was under the impression that operator was always a JavaScript comparison operator like !==. A function name is surprising to me. I'm guessing it's too late to prevent such a change from ending up in a release. (Maybe I'm wrong and 'throws' has been a legit value for operator for years?)

I bring this up because I'd rather operator be undefined than fail unless there's a compelling reason to make it the function name sometimes but not all the time. That just seems odd to me. But my argument there is less compelling as we've already started going down this road apparently.

@BridgeAR
Copy link
Member Author

BridgeAR commented Sep 8, 2018

@Trott I am not sure anymore since when that is the case. I think since v8 or v9. Using e.g., === would have been wrong as that is not the actual used comparison. That is why they changed to the function name. Only the loose equal checks contain == and != as operator, plus assert.ok() and partially assert.fail(). Since we filter the function name from the stack trace, it's good to know what function actually triggered the assertion. That's also why they changed to the function name and why I added it here as well.

@Trott
Copy link
Member

Trott commented Sep 8, 2018

@BridgeAR Thanks. Just to be clear: Not objecting. Just surprised. :-D

@BridgeAR
Copy link
Member Author

PTAL this could use some reviews.

lib/assert.js Outdated
actual,
expected,
message,
operator,
operator: operator || 'fail',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does Node have a consistent way of checking for optional params or default params?
In this case the falsey check means that users can't specify it as null or "".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's often mixed and fasly values are often set to a default while in other cases only undefined is set to a default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method has a mix of deprecated/legacy and new/different functionality. Would moving the operator juggle into a argLength < 2 branch with the other argLen checks be doable? That way it touches less of the overlapping code paths.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was done on purpose since I believe it is best to just properly do this for all cases.

Shall I maybe change the check to: operator === undefined ? 'fail' : operator?

@BridgeAR
Copy link
Member Author

PTAL

@@ -106,13 +107,23 @@ function fail(actual, expected, message, operator, stackStartFn) {
operator = '!=';
}

innerFail({
if (message instanceof Error) throw message;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

☝️ Should use the util for isError (internal reference) for this since it avoids the error of errors from other realms.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only changing this here is likely not the best way. It would have to be changed throughout assert in total or not at all. It is also sad that the util function is not a safe check as the Symbol.toStringTag could be set and isError would happily return true in some weird cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only changing this here is likely not the best way.

Ah, didn't know this was consistent with the rest of assert. A follow up would be best.

It is also sad that the util function is not a safe check as the Symbol.toStringTag could be set and isError would happily return true in some weird cases.

Oh I thought the new isNativeError was more robust.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I missed the isNativeError somehow. We have multiple places in core where we should likely switch to this check.

@BridgeAR
Copy link
Member Author

PTAL

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 17, 2018
@BridgeAR
Copy link
Member Author

BridgeAR added a commit to BridgeAR/node that referenced this pull request Sep 17, 2018
This makes sure `assert.fail()` contains an operator instead of being
undefined.

On top of that it also fixes the `err.generatedMessage` property.
Before, it was not always set correct.

PR-URL: nodejs#22694
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@BridgeAR
Copy link
Member Author

Landed in eee5adf 🎉

@BridgeAR BridgeAR closed this Sep 17, 2018
targos pushed a commit that referenced this pull request Sep 18, 2018
This makes sure `assert.fail()` contains an operator instead of being
undefined.

On top of that it also fixes the `err.generatedMessage` property.
Before, it was not always set correct.

PR-URL: #22694
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Sep 19, 2018
This makes sure `assert.fail()` contains an operator instead of being
undefined.

On top of that it also fixes the `err.generatedMessage` property.
Before, it was not always set correct.

PR-URL: #22694
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this pull request Sep 20, 2018
This makes sure `assert.fail()` contains an operator instead of being
undefined.

On top of that it also fixes the `err.generatedMessage` property.
Before, it was not always set correct.

PR-URL: #22694
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@BridgeAR BridgeAR deleted the improve-assert-fail branch January 20, 2020 11:38
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.

5 participants