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 <teddy.katz@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Remove string literal message in assert.strictEqual() calls in test-async-await.js. Backport-PR-URL: #22912 PR-URL: #22849 Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <teddy.katz@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <teddy.katz@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <teddy.katz@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <teddy.katz@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Remove string literal from assert.strictEqual message to improve output of AssertionError. Backport-PR-URL: #22912 PR-URL: #22849 Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Remove unnecessary string literal from assert.deepStrictEqual() call. Backport-PR-URL: #22912 PR-URL: #22849 Reviewed-By: Teddy Katz <teddy.katz@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <teddy.katz@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <teddy.katz@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <teddy.katz@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <teddy.katz@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <daniel.bevenius@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <teddy.katz@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <teddy.katz@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <teddy.katz@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <teddy.katz@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <teddy.katz@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <teddy.katz@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <teddy.katz@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Trott commented Sep 13, 2018

nodejs-github-bot commented Sep 13, 2018

Trott commented Sep 13, 2018

BridgeAR left a comment

mscdex left a comment

Trott commented Sep 14, 2018

mcollina commented Sep 14, 2018

mcollina left a comment

addaleax commented Sep 14, 2018

Trott commented Sep 14, 2018

Trott commented Sep 14, 2018 •

edited

Loading

BridgeAR commented Sep 14, 2018

Trott commented Sep 14, 2018

refack commented Sep 14, 2018

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

Conversation

Trott commented Sep 13, 2018

Checklist

nodejs-github-bot commented Sep 13, 2018

Trott commented Sep 13, 2018

BridgeAR left a comment

Choose a reason for hiding this comment

mscdex left a comment

Choose a reason for hiding this comment

Trott commented Sep 14, 2018

mcollina commented Sep 14, 2018

mcollina left a comment

Choose a reason for hiding this comment

addaleax commented Sep 14, 2018

Trott commented Sep 14, 2018

Trott commented Sep 14, 2018 • edited Loading

BridgeAR commented Sep 14, 2018

Trott commented Sep 14, 2018

refack commented Sep 14, 2018

Trott commented Sep 14, 2018 •

edited

Loading