tools, test: forbid string literal as third argument for assert.strictEqual() and friends#22849

Trott · 2018-09-13T21:47:11Z

String literals provided as the third argument to assert.strictEqual() or assert.deepStrictEqual() will mask the values that are causing issues. Use a lint rule to prevent such usage.

@nodejs/testing @nodejs/linting

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Remove string literal message in assert.strictEqual() call in napi test testFinalizer.js.

Remove string literal message in assert.strictEqual() calls in test-async-await.js.

Remove an unecessary string literal from assert.strictEqual() call in test-inspector.js. The string literal is printed instead of the value that causes an error. Removing the string literal allows the value that caused the error to be printed. This improves the troubleshooting experience when the test fails due to that assertion.

In test-http2-timeout-large-write.js and test-http2-timeout-large-write-file.js: Use assert.ok() on a boolean that the test itself creates and sets, rather than assert.strictEqual(). This allows us to use a static message without running afoul of the upcoming "do not use string literals with assert.strictEqual()" lint rule.

Refactor test-vm-run-in-new-context so that check for `--expose-gc` flag will not run afoul of an upcoming lint rule that checks that string literals are not used for the `message` argument of `assert.strictEqual()`.

Remove string literal from `assert.strictEqual()` call `message` parameter and make it a comment above the assertion instead.

Remove string literal from assert.strictEqual message to improve output of AssertionError.

Remove unnecessary string literal from assert.deepStrictEqual() call.

Remove string literal as assertion message in call to assert.strictEqual() in test-dns-resolveany-bad-ancount.

Remove string literal as assertion message in call to assert.strictEqual() in test-dns-lookup.

Make minor modifications to test-assert.js to prepare it for linting rule that forbids the use of string literals for the third argument of assert.strictEqual().

String literals provided as the third argument to assert.strictEqual() or assert.deepStrictEqual() will mask the values that are causing issues. Use a lint rule to prevent such usage.

nodejs-github-bot · 2018-09-13T21:47:12Z

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

Trott · 2018-09-13T21:48:11Z

CI: https://ci.nodejs.org/job/node-test-pull-request/17177/

BridgeAR

I recently opened a PR to make use of the third argument. Prohibiting it seems the wrong way for me.

mscdex

I don't think we should forbid this. I could see some complicated situations where given enough detail in the custom message, a different error message could make things easier to understand what went wrong.

Trott · 2018-09-14T04:52:52Z

I don't think we should forbid this. I could see some complicated situations where given enough detail in the custom message, a different error message could make things easier to understand what went wrong.

@mcollina This doesn't forbid messages. It forbids string literals as messages as those are (almost) always anti-patterns. And in the unusual instances where a string literal makes sense, the rule can be disabled with a comment, just like we do for the occasional instance where it makes sense for a line to stretch out for more than 80 characters.

mcollina · 2018-09-14T06:51:00Z

@Trott you tagged me in place of @mscdex

mcollina

LGTM

addaleax · 2018-09-14T16:08:02Z

I could see some complicated situations where given enough detail in the custom message, a different error message could make things easier to understand what went wrong.

@mscdex Just to be clear, this isn’t forbidding generating more complex messages – it forbids literals, i.e. purely static messages, because they nearly always obfuscate information.

Trott · 2018-09-14T18:53:30Z

@Trott you tagged me in place of @mscdex

Doh! Sorry about my error.

Trott · 2018-09-14T18:55:41Z

I recently opened a PR to make use of the third argument. Prohibiting it seems the wrong way for me.

@BridgeAR My preference would be:

Introduce this on master and backport to supported release lines.
Remove this from branches if and when your proposed change to message generation lands on each branch.

Does that work for you?

BridgeAR · 2018-09-14T19:14:16Z

Yes, that works for me.

Outdated.

Trott · 2018-09-14T19:26:11Z

@mscdex Does this alleviate your concerns?

This doesn't forbid messages. It forbids string literals as messages as those are (almost) always anti-patterns. And in the unusual instances where a string literal makes sense, the rule can be disabled with a comment, just like we do for the occasional instance where it makes sense for a line to stretch out for more than 80 characters.

refack · 2018-09-14T19:49:48Z

Since this PR "eats" human written institutional knowledge, I'm going to be petty, and ask to see the before and after message for those changes, so we can make sure we don't lose anything.

Remove string literal message in assert.strictEqual() call in napi test testFinalizer.js. Backport-PR-URL: #22912 PR-URL: #22849 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Remove string literal message in assert.strictEqual() calls in test-async-await.js. Backport-PR-URL: #22912 PR-URL: #22849 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Remove an unecessary string literal from assert.strictEqual() call in test-inspector.js. The string literal is printed instead of the value that causes an error. Removing the string literal allows the value that caused the error to be printed. This improves the troubleshooting experience when the test fails due to that assertion. Backport-PR-URL: #22912 PR-URL: #22849 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

In test-http2-timeout-large-write.js and test-http2-timeout-large-write-file.js: Use assert.ok() on a boolean that the test itself creates and sets, rather than assert.strictEqual(). This allows us to use a static message without running afoul of the upcoming "do not use string literals with assert.strictEqual()" lint rule. Backport-PR-URL: #22912 PR-URL: #22849 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Refactor test-vm-run-in-new-context so that check for `--expose-gc` flag will not run afoul of an upcoming lint rule that checks that string literals are not used for the `message` argument of `assert.strictEqual()`. Backport-PR-URL: #22912 PR-URL: #22849 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Remove string literal from `assert.strictEqual()` call `message` parameter and make it a comment above the assertion instead. Backport-PR-URL: #22912 PR-URL: #22849 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Remove string literal from assert.strictEqual message to improve output of AssertionError. Backport-PR-URL: #22912 PR-URL: #22849 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Remove unnecessary string literal from assert.deepStrictEqual() call. Backport-PR-URL: #22912 PR-URL: #22849 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Remove string literal as assertion message in call to assert.strictEqual() in test-dns-resolveany-bad-ancount. Backport-PR-URL: #22912 PR-URL: #22849 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Remove string literal as assertion message in call to assert.strictEqual() in test-dns-lookup. Backport-PR-URL: #22912 PR-URL: #22849 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Make minor modifications to test-assert.js to prepare it for linting rule that forbids the use of string literals for the third argument of assert.strictEqual(). Backport-PR-URL: #22912 PR-URL: #22849 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

String literals provided as the third argument to assert.strictEqual() or assert.deepStrictEqual() will mask the values that are causing issues. Use a lint rule to prevent such usage. Backport-PR-URL: #22912 PR-URL: #22849 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

In short: Some unit tests are using string literals to simply tell you a conclusion what's right/wrong BUT not tell you what actually values are. So it's necessary to print them out in the console. Refs: #22849 PR-URL: #22891 Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: James M Snell <[email protected]>

Remove an unecessary string literal from assert.strictEqual() call in test-inspector.js. The string literal is printed instead of the value that causes an error. Removing the string literal allows the value that caused the error to be printed. This improves the troubleshooting experience when the test fails due to that assertion. Backport-PR-URL: #22888 PR-URL: #22849 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

In test-http2-timeout-large-write.js and test-http2-timeout-large-write-file.js: Use assert.ok() on a boolean that the test itself creates and sets, rather than assert.strictEqual(). This allows us to use a static message without running afoul of the upcoming "do not use string literals with assert.strictEqual()" lint rule. Backport-PR-URL: #22888 PR-URL: #22849 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Refactor test-vm-run-in-new-context so that check for `--expose-gc` flag will not run afoul of an upcoming lint rule that checks that string literals are not used for the `message` argument of `assert.strictEqual()`. Backport-PR-URL: #22888 PR-URL: #22849 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Remove string literal from `assert.strictEqual()` call `message` parameter and make it a comment above the assertion instead. Backport-PR-URL: #22888 PR-URL: #22849 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Remove string literal as assertion message in call to assert.strictEqual() in test-dns-resolveany-bad-ancount. Backport-PR-URL: #22888 PR-URL: #22849 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Make minor modifications to test-assert.js to prepare it for linting rule that forbids the use of string literals for the third argument of assert.strictEqual(). Backport-PR-URL: #22888 PR-URL: #22849 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

In test-http2-timeout-large-write.js and test-http2-timeout-large-write-file.js: Use assert.ok() on a boolean that the test itself creates and sets, rather than assert.strictEqual(). This allows us to use a static message without running afoul of the upcoming "do not use string literals with assert.strictEqual()" lint rule. Backport-PR-URL: nodejs#22912 PR-URL: nodejs#22849 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Trott added 12 commits September 13, 2018 14:00

test: remove string literal message from assertion
abcaf56
Remove string literal message in assert.strictEqual() call in napi test testFinalizer.js.

test: remove string literal message in assertions
486f6e8
Remove string literal message in assert.strictEqual() calls in test-async-await.js.

test: refactor flag check
b23cb6c
Refactor test-vm-run-in-new-context so that check for `--expose-gc` flag will not run afoul of an upcoming lint rule that checks that string literals are not used for the `message` argument of `assert.strictEqual()`.

test: remove string literal from assertion
36769be
Remove string literal from `assert.strictEqual()` call `message` parameter and make it a comment above the assertion instead.

test: remove string literal message from assertion
26f2e99
Remove string literal from assert.strictEqual message to improve output of AssertionError.

test: remove string literal arg from assertion
798a6dd
Remove unnecessary string literal from assert.deepStrictEqual() call.

test: remove string literal from assertion
af1f7c7
Remove string literal as assertion message in call to assert.strictEqual() in test-dns-resolveany-bad-ancount.

test: remove string literal from assertion
8572e6e
Remove string literal as assertion message in call to assert.strictEqual() in test-dns-lookup.

test: prepare test-assert for strictEqual linting
4b9142c
Make minor modifications to test-assert.js to prepare it for linting rule that forbids the use of string literals for the third argument of assert.strictEqual().

tools: prevent string literals in some assertions
51cd1ea
String literals provided as the third argument to assert.strictEqual() or assert.deepStrictEqual() will mask the values that are causing issues. Use a lint rule to prevent such usage.

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Sep 13, 2018

not-an-aardvark approved these changes Sep 13, 2018
View reviewed changes

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 13, 2018

BridgeAR previously requested changes Sep 13, 2018
View reviewed changes

mscdex previously requested changes Sep 14, 2018
View reviewed changes

mcollina approved these changes Sep 14, 2018
View reviewed changes

addaleax approved these changes Sep 14, 2018
View reviewed changes

BethGriggs mentioned this pull request Oct 30, 2018
v8.13.0 proposal #23974
Merged

beevelop mentioned this pull request Nov 20, 2018
2018-11-20 Version 8.13.0 'Carbon' (LTS) @MylesBorins prepared by @BethGriggs beevelop/docker-android-nodejs#250
Closed

Trott deleted the strictequal-literal branch January 13, 2022 22:50

aduh95 mentioned this pull request Nov 13, 2024
test: ensure that CLI options are alphabetical #55790
Closed

LiviaMedeiros mentioned this pull request Nov 24, 2025
test: add lint rule to forbid use of assert.ok(/regex/.test(…))#60832
Merged

Uh oh!

tools, test: forbid string literal as third argument for assert.strictEqual() and friends#22849

tools, test: forbid string literal as third argument for assert.strictEqual() and friends #22849

Uh oh!

Conversation

Trott commented Sep 13, 2018

Checklist

Uh oh!

nodejs-github-bot commented Sep 13, 2018

Uh oh!

Trott commented Sep 13, 2018

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

mscdex left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented Sep 14, 2018

Uh oh!

mcollina commented Sep 14, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 14, 2018

Uh oh!

Trott commented Sep 14, 2018

Uh oh!

Trott commented Sep 14, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Sep 14, 2018

Uh oh!

Trott commented Sep 14, 2018

Uh oh!

refack commented Sep 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

Trott commented Sep 14, 2018•
edited
Loading