assert: fix internal logic judgment by zero1five · Pull Request #27743 · nodejs/node

zero1five · 2019-05-16T23:06:10Z

Fix the logic judgment of assert.fail and add doc
and the test cases for the performance of assert.fail
on multiple parameter.

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

zero1five · 2019-05-18T12:12:11Z

review wanted.
If I miss something, please point. thank you very much!

Trott · 2019-05-19T04:19:55Z

I'm honestly not sure this isn't a bug and the behavior should be as described in the docs currently. But I'm also not sure it's worth changing/"fixing" even if that's the case. This looks good to me, but maybe @BridgeAR or @refack would want to take a look?

BridgeAR

The code change does not seem right to me. All added test cases work identical as before with this patch besides the last one which should ideally print 'Failed' which it currently does.

The documentation could use an update though. My plane is about to take off so I'll review that later.

refack · 2019-05-19T16:44:08Z

Hello @zero1five, and thank you for the contribution!
I'm not sure I have anything of significance to contribute, except that since this makes changes around a deprecated signature, I'd like the distinction between the supported and deprecated signatures be as explicit as possible.

BridgeAR · 2019-05-19T20:26:58Z

@zero1fiveassert.fail(undefined) should result in 'Failed' as outlined above. Resulting in 'undefined fail undefined' is not ideal at all. The default operator should also be != and not 'fail' in case the operator is undefined but more than a single argument is passed through.

zero1five · 2019-05-20T00:58:35Z

@BridgeAR Thank you very much for your review! I made some changes, can you please review it?

BridgeAR

Thanks for sticking to it! I left a few more comments.

Fix the logic judgment of assert.fail and add doc and the test cases for the performance of assert.fail on multiple parameter.

BridgeAR · 2020-01-12T13:32:27Z

@zero1five sorry that it took a while to look at this again. Some times it's difficult to keep an overview over all the PRs.

This needs a rebase but I think this should otherwise be good to go.

@Trott would you please take another look at the docs?

Trott · 2020-01-26T10:47:16Z

-given, the default message `Failed` will be used.
+`expected` separated by the provided `operator`. If `operator` is falsy and no
+message is set, it'll default to `'!='`. If `message` is truthy the `operator`
+will default to `'fail'` and `message` will be used as error message. Other


Suggested change
will default to `'fail'` and `message` will be used as error message. Other
will default to `'fail'` and `message` will be used for the error message. Other

Trott · 2020-01-26T10:48:59Z

@Trott would you please take another look at the docs?

I left some suggestions but they're all nits, This can land without them,.

Trott · 2020-01-26T10:55:32Z

+ actual: undefined,
+ expected: undefined,
+ generatedMessage: true
+});


This test currently passes without the other changes in this PR. Can we please add a test case that currently fails?

Trott

Needs a test case that fails on current master branch.

We should probably come up with a more detailed commit message to use while landing. (@BridgeAR: Any thoughts on that?)

HarshithaKP · 2020-04-24T06:50:35Z

@zero1five, there are few review comments and this needs a rebase.

BridgeAR · 2020-04-26T22:43:52Z

 if (message instanceof Error) throw message;

+ if (operator === undefined) {
+ if (message) {


Suggested change
if(message){
if(message!=null){

BridgeAR · 2020-04-26T22:54:54Z

+ assert.fail(undefined);
+}, {
+ code: 'ERR_ASSERTION',
+ name: 'AssertionError',
+ message: 'Failed',
+ operator: 'fail',
+ actual: undefined,
+ expected: undefined,


Suggested change
assert.fail(undefined);
},{
code: 'ERR_ASSERTION',
name: 'AssertionError',
message: 'Failed',
operator: 'fail',
actual: undefined,
expected: undefined,
assert.fail(1,2,undefined,undefined);
},{
code: 'ERR_ASSERTION',
name: 'AssertionError',
message: '1 != 2',
operator: '!=',
actual: 1,
expected: 2,
Ideally, another identical test could be added that sets message to null.

BridgeAR · 2020-04-26T23:04:41Z

+`expected` separated by the provided `operator`. If `operator` is falsy and no
+message is set, it will default to `'!='`. If `message` is truthy, the `operator`
+will default to `'fail'` and `message` will be used for the error message. Other
+arguments will be stored as properties on the thrown object. If `stackStartFn`
+is provided, all stack frames above that function will be removed from
+stacktrace (see [`Error.captureStackTrace`][]).


I suggest to change the operator description above to explain when it's set to 'fail' and when to '!='.
I would also move the stackStartFn description to the part above. That way the total description below concentrates on the general behavior instead of describing each individual option setting.
Right now we also miss to document that assert.fail(1, 2, new Error() throws the passed through error. That could be described in the message property above.

The comments should be addressed before this lands.

jasnell · 2020-07-07T14:54:54Z

This would need a rebase in order to move forward

nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label May 16, 2019

Trott requested a review from BridgeAR May 17, 2019 21:03

Trott reviewed May 19, 2019
View reviewed changes

Comment threaddoc/api/assert.md Outdated

BridgeAR reviewed May 19, 2019
View reviewed changes

zero1five force-pushed the fix/assert-fail-api branch 2 times, most recently from cde5202 to dfe1904Compare May 19, 2019 08:17

zero1five force-pushed the fix/assert-fail-api branch from dfe1904 to 9553cafCompare May 20, 2019 00:55

zero1five force-pushed the fix/assert-fail-api branch from 7cb6eb0 to f8314d6Compare May 21, 2019 12:08

BridgeAR reviewed May 21, 2019
View reviewed changes

Comment threadlib/assert.js Outdated
Comment threadlib/assert.js Outdated
Comment threaddoc/api/assert.md Outdated

zero1five force-pushed the fix/assert-fail-api branch from f8314d6 to 7994c95Compare May 22, 2019 16:45

assert: fix internal logic judgment
b87a772
Fix the logic judgment of assert.fail and add doc and the test cases for the performance of assert.fail on multiple parameter.

zero1five force-pushed the fix/assert-fail-api branch from 5534055 to b87a772Compare May 22, 2019 17:03

zero1five added 2 commits May 23, 2019 08:43

assert: fix eslint rule
4eb0f24

assert: extract deprecated information to the function
a9f7f54

Trott force-pushed the master branch from 1ecc406 to 49cf67eCompare September 17, 2019 16:51

devnexen force-pushed the master branch from e8a4568 to 5289f80Compare December 26, 2019 19:46

BridgeAR previously approved these changes Jan 12, 2020
View reviewed changes

Trott reviewed Jan 26, 2020
View reviewed changes

Comment threaddoc/api/assert.md Outdated

Trott reviewed Jan 26, 2020
View reviewed changes

Comment threaddoc/api/assert.md Outdated

Trott reviewed Jan 26, 2020
View reviewed changes

Comment threaddoc/api/assert.md Outdated

Trott reviewed Jan 26, 2020
View reviewed changes

Trott requested changes Jan 26, 2020
View reviewed changes

Trott added 3 commits April 26, 2020 14:52

Update doc/api/assert.md
cd85b5b

Update doc/api/assert.md
21a9f93

Update doc/api/assert.md
24e9212

BridgeAR reviewed Apr 26, 2020
View reviewed changes

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72Compare May 31, 2020 12:19

jasnell added the stalled Issues and PRs that are stalled. label Jul 7, 2020

jasnell closed this Jul 7, 2020

	will default to `'fail'` and `message` will be used as error message. Other
	will default to `'fail'` and `message` will be used for the error message. Other

Uh oh!

Uh oh!

Conversation

zero1five commented May 16, 2019

Checklist

Uh oh!

zero1five commented May 18, 2019

Uh oh!

Uh oh!

Trott commented May 19, 2019

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented May 19, 2019

Uh oh!

BridgeAR commented May 19, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zero1five commented May 20, 2019

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BridgeAR commented Jan 12, 2020

Uh oh!

Uh oh!

Uh oh!

TrottJan 26, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Trott commented Jan 26, 2020

Uh oh!

TrottJan 26, 2020

Choose a reason for hiding this comment

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

HarshithaKP commented Apr 24, 2020

Uh oh!

BridgeARApr 26, 2020

Choose a reason for hiding this comment

Uh oh!

BridgeARApr 26, 2020

Choose a reason for hiding this comment

Uh oh!

BridgeARApr 26, 2020

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jul 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

BridgeAR commented May 19, 2019•
edited
Loading