assert: combine user and generated message #22695

BridgeAR · 2018-09-04T23:01:02Z

There are lots of PRs that remove the message property assert calls. That message is actually pretty valuable and should not be removed. However, the message alone is often not that useful. In context with the auto-generated message it can be very valuable.
From now on it would also be possible to provide the actual input value to functions and similar instead of having to reconstruct what value the assertion failed upon:

assert.strictEqual(foobar(input), 'output', input)
assert.ok(/regexp/.test(input), input)

Before, all values were coerced to a string and passing through a message would have resulted in [object Object] as output. Now the value is inspected and fully visible.

There is only one values that is not accepted as input: instances of Error (those are special handled in the way that that concrete error would be thrown in case of an failed assertion. Therefore even if someone does not anticipates this case, it's always a good assertion output). I have a follow-up PR to add support for undefined as well.

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 · 2018-09-04T23:01:03Z

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/795/pipeline

Trott · 2018-09-05T02:07:47Z

Seems like the documentation changes could benefit from a bit of editing. @nodejs/documentation

Trott · 2018-09-05T05:48:43Z

What about instances where expected and actual are very long values and the use of the message to suppress them may therefore be desirable?

vsemozhetbyt · 2018-09-05T07:36:14Z

doc/api/deprecations.md

+
+Type: Runtime
+
+Instead of accessing the generatedMessage property of `AssertionErrors`, please


generatedMessage -> `generatedMessage`

vsemozhetbyt · 2018-09-05T07:37:41Z

doc/api/deprecations.md

+Type: Runtime
+
+Instead of accessing the generatedMessage property of `AssertionErrors`, please
+check for `AssertionError#userMessage` existence instead. It contains the actual


Is double "Instead of... instead." OK in this sentence?

vsemozhetbyt · 2018-09-05T07:43:52Z

doc/api/assert.md

 * `operator` {string} Set to the passed in operator value.
+* `userMessage` {any} Contains the actual passed through message by the user.
+  It will not be set in case the user does not provide a error message.


a error -> an error

BridgeAR · 2018-09-05T08:31:55Z

@Trott

What about instances where expected and actual are very long values and the use of the message to suppress them may therefore be desirable?

The actual and expected values will be cut off by default if they are very long. Especially, if there are only few differences in the diff (collapsing of matching parts). Therefore I do not worry about the extra lines.

BridgeAR · 2018-09-05T08:45:47Z

CITGM https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1533/

fastify has test failures due to sniffing the message. I'll open a PR to fix those. The rest seems green.

addaleax · 2018-09-05T11:17:11Z

doc/api/assert.md

@@ -74,6 +93,7 @@ try {
  assert.strictEqual(err.code, 'ERR_ASSERTION');
  assert.strictEqual(err.operator, 'strictEqual');
  assert.strictEqual(err.generatedMessage, true);
+  assert.strictEuqal('userMessage' in err, false);


typo: strictEqual

BridgeAR · 2018-09-05T22:40:53Z

PTAL

This PR makes all the "remove third argument in assert calls" obsolete.

Refs: #22718

BridgeAR · 2018-09-06T00:32:38Z

I opened a PR to fix the fastify issues. Refs: fastify/fastify#1142

Trott · 2018-09-06T06:05:47Z

Might be worth pinging some mocha people (@boneskull) and code/lab people (@cjihrig, @geek) to see if this is likely to have any impact on users of their libraries. (I'm guessing not, but let's be cautious anyway.)

mcollina

I don’t understand why this PR would cause such a breakage. That seems a very common pattern, and not something that would be specific to Fastify. I suspect this might be breaking more.

BridgeAR · 2018-09-06T09:11:13Z

@fhinkel all semver major PRs need review by the TSC. Thus, I added the request for review.

@mcollina this is indeed not specific to fastify, even tough I am pretty happy to see that CITGM did not find anything else. It is almost only an issue for the following case:

assert is used in production code including a custom message. This message is then tested for in their tests to verify everything worked fine.

I am going to split this into smaller steps to (a) make the userMessage backportable (that should be relied upon programmatically later on) (b) to accept further types in the message before and should be backported, which leads to (c) this change would be significantly smaller and easier to review.

I am also going to check the usage with Gzemnid even tough most entries will likely be false positives and filtering them is not that easy / requires a lot of manual work.

Adding the blocked label until I split this into the three pieces.

cjihrig · 2018-09-06T13:23:01Z

This won't impact code or lab. Thank you for checking though!

So far it was difficult to tell what the actual error was about if there was only the user error. To overcome this, the user message and the auto-generated message will now be visible at the same time.

Trott · 2018-11-28T03:34:30Z

@BridgeAR Still want to see this one land If so, can you rebase?

@mcollina Anything that can be changed to make this acceptable to you? Or is your opposition to it a hard -1? (Personally, I'm undecided. I can see the reasons for doing it and I can see the reasons for not changing it.)

BridgeAR · 2018-12-19T17:21:02Z

Closing for now. I'll open a new PR when I get to it.

boneskull · 2018-12-22T00:50:33Z

user installs of Mocha are not impacted directly by changes to assert, so do whatever.

nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Sep 4, 2018

BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 4, 2018

BridgeAR requested review from benjamingr, jasnell, addaleax, mcollina, Trott and a team September 4, 2018 23:08

BridgeAR mentioned this pull request Sep 4, 2018

assert: accept undefined as message input #22696

Closed

4 tasks

vsemozhetbyt reviewed Sep 5, 2018

View reviewed changes

vsemozhetbyt added the deprecations Issues and PRs related to deprecations. label Sep 5, 2018

addaleax reviewed Sep 5, 2018

View reviewed changes

BridgeAR requested a review from a team September 5, 2018 19:20

fhinkel removed the request for review from a team September 6, 2018 04:33

mcollina requested changes Sep 6, 2018

View reviewed changes

BridgeAR added the blocked PRs that are blocked by other issues or PRs. label Sep 6, 2018

assert: combine auto generated message with user message

89a9f00

So far it was difficult to tell what the actual error was about if there was only the user error. To overcome this, the user message and the auto-generated message will now be visible at the same time.

BridgeAR force-pushed the combine-user-and-generated-message branch from 8f9d266 to 89a9f00 Compare October 3, 2018 19:15

BridgeAR closed this Dec 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assert: combine user and generated message #22695

assert: combine user and generated message #22695

BridgeAR commented Sep 4, 2018

nodejs-github-bot commented Sep 4, 2018

Trott commented Sep 5, 2018

Trott commented Sep 5, 2018

vsemozhetbyt Sep 5, 2018

vsemozhetbyt Sep 5, 2018

vsemozhetbyt Sep 5, 2018

BridgeAR commented Sep 5, 2018

BridgeAR commented Sep 5, 2018

addaleax Sep 5, 2018

BridgeAR commented Sep 5, 2018

BridgeAR commented Sep 6, 2018

Trott commented Sep 6, 2018

mcollina left a comment

BridgeAR commented Sep 6, 2018

cjihrig commented Sep 6, 2018

Trott commented Nov 28, 2018

BridgeAR commented Dec 19, 2018

boneskull commented Dec 22, 2018


		Type: Runtime

		Instead of accessing the generatedMessage property of `AssertionErrors`, please

assert: combine user and generated message #22695

assert: combine user and generated message #22695

Conversation

BridgeAR commented Sep 4, 2018

Checklist

nodejs-github-bot commented Sep 4, 2018

Trott commented Sep 5, 2018

Trott commented Sep 5, 2018

vsemozhetbyt Sep 5, 2018

Choose a reason for hiding this comment

vsemozhetbyt Sep 5, 2018

Choose a reason for hiding this comment

vsemozhetbyt Sep 5, 2018

Choose a reason for hiding this comment

BridgeAR commented Sep 5, 2018

BridgeAR commented Sep 5, 2018

addaleax Sep 5, 2018

Choose a reason for hiding this comment

BridgeAR commented Sep 5, 2018

BridgeAR commented Sep 6, 2018

Trott commented Sep 6, 2018

mcollina left a comment

Choose a reason for hiding this comment

BridgeAR commented Sep 6, 2018

cjihrig commented Sep 6, 2018

Trott commented Nov 28, 2018

BridgeAR commented Dec 19, 2018

boneskull commented Dec 22, 2018