test: add common.mustSucceed #35086

tniessen · 2020-09-07T01:32:28Z

Our increasingly large collection of tests makes frequent use of this pattern:

someFunction(/* some args */, common.mustCall((err, /* some results */) => {
  assert.ifError(err);
  // Something else
});

This PR changes those to this pattern:

someFunction(/* some args */, common.mustSucceed((/* some results */) => {
  // Something else
});

This simple change allows us to remove more than 300 lines of code from tests. Another aspect is that many tests really should be using mustSucceed() instead of mustCall(), because mustCall() silently ignores errors passed to it. I changed the crypto tests manually where I found such cases, and a few other test files, but there's probably dozens or hundreds of such cases in other test files.

To make this easier to review, I separated the changes into multiple commits.

The first commit introduces the new API.
The next three commits apply regular expressions for automatic conversions from mustCall to mustSucceed.
1. The first replaces all occurrences of mustCall$[ \n]*(\(erro?r?$[ \n]*=>[ \n]*\{?|function[ \n]*$erro?r?$[ \n]*\{)?[ \n]*assert\.ifError($erro?r?$)?;?[ \n]*\}?[ \n]*\) with mustSucceed(). These are trivial cases, e.g, mustCall(assert.ifError), mustCall((err) => assert.ifError(err)), mustCall(function(err) { assert.ifError(err); }) etc.
2. The second replaces all occurrences of mustCall$[\n ]*\(erro?r?(,[\n ]*([^$]*))?\)([\n ]*)=>([\n ]*)\{[\n ]+assert\.ifError$erro?r?$;\n with mustSucceed(($2)$3=>$4{\n. These are arrow functions that take an error argument and begin with assert.ifError.
3. The third replaces all occurrences of mustCall$[\n ]*function[ \n]*\(erro?r?(, *([^$]*))?\)([ \t\n]*)\{[\n ]+assert\.ifError$erro?r?$;\n with mustSucceed(function($2)$3{\n. These are regular functions that take an error argument and begin with assert.ifError.
The fifth commit converts functions containing statements such as assert.ok(!err) to mustSucceed, where possible.
The sixth commit improves formatting. While all previous commits pass tests and linter, the regular expressions leave some unnecessary empty lines. While at it, I also converted some functions to arrow functions (only in lines that I changed anyway).
The seventh commit replaces the weaker mustCall with mustSucceed where it seems to make sense.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Trott · 2020-09-07T01:38:42Z

My immediate reaction is that I prefer the explicitness and clarity of the current pattern. common.mustSucceed() hides just a bit too much for my tastes, and both brevity and lack of repetition in tests are overrated. (Lack of repetition is a virtue almost everywhere else, though.)

But that's my reaction without looking much and I have to admit you make some good points in your explanation. I'll take a closer look later, but for now: /ping @nodejs/testing

Trott · 2020-09-07T03:26:34Z

My immediate reaction is that I prefer the explicitness and clarity of the current pattern.

Having spent a little more time with this, I think I'm a convert. This seems like a good idea to me.

Trott

The new function needs to be documented in test/common/README.md. Also, might be worthwhile looking for common.mustCall() instances in our internal docs that need to be changed. I'm thinking specifically of doc/guides/writing-tests.md but there may be others.

meh, not really blocking

lpinca · 2020-09-07T05:13:24Z

My immediate reaction is that I prefer the explicitness and clarity of the current pattern.

I agree, and I think this does not help much with cases where mustCall() ignores the error. Even if this is added the test writer could use mustCall() instead of mustSucceed(). If reviewers notice that the error is ignored they can ask the test writer to use mustSucceed() but at this point, isn't it easier and clearer if they ask the test writer to use assert.ifError(err);?

It's test writer responsibility to make sure errors are not ignored with or without this change.

tniessen · 2020-09-07T10:11:16Z

My immediate reaction is that I prefer the explicitness and clarity of the current pattern. common.mustSucceed() seems be just a bit too magical for my tastes, and both brevity and lack of repetition in tests are overrated.

Personally, I think that the callback(err) pattern is such a natural part of Node.js that we should not treat err as a regular argument. In most tests, its value has absolutely no use, except that the test should fail if it is not null or undefined (and many tests do not implement that). mustSucceed would simply reinforce the pattern of seeing err as an exception, not a regular argument.

We have something remotely similar for Promise APIs, assert.doesNotReject, and we came to the conclusion that it isn't helpful. For callback APIs, we have absolutely no test utility, even though it would be helpful.

I think this does not help much with cases where mustCall() ignores the error. Even if this is added the test writer could use mustCall() instead of mustSucceed().

From my perspective, simply having mustSucceed() and beginning to use it would have a chance of counteracting the fact that many tests use mustCall() incorrectly.

If reviewers notice that the error is ignored they can ask the test writer to use mustSucceed() but at this point, isn't it easier and clearer if they ask the test writer to use assert.ifError(err);?

I actually started this because I noticed a lot of tests using common.mustCall() without an error handler as a callback that receives an err. Personally, I think having mustSucceed() as a shorthand for mustCall(assert.ifError) is neither less readable nor understandable. From there, extending it to functions that do their own validation seemed to make sense.

tniessen · 2020-09-07T10:58:18Z

Kindly asking @jasnell, @BridgeAR, @cjihrig, @addaleax for input due to their experience with these particular tests.

$ git rev-parse HEAD
1adb4cd0b18fa9dac257def9dbeabe7e687be1b0
$ git rev-parse master
bb9117ee9f8fb42ac8efd1dab30b3faee5463587
$ git log --format='%aN' master -- $(git diff master --name-only) | sort | uniq -c | sort -r | head -n5
    138 Rich Trott
     75 James M Snell
     72 Ruben Bridgewater
     51 Colin Ihrig
     47 Anna Henningsen

addaleax · 2020-09-07T11:43:43Z

My initial thought would be that this only really makes sense if it is enforced through a linter rule, because I would expect that this pattern does look surprising to people who are new to our test suite (the same obviously goes for most patterns that are specific to it).

tniessen · 2020-09-07T13:19:01Z

My initial thought would be that this only really makes sense if it is enforced through a linter rule

I think we could use a linter rule to prevent common.mustCall((err) => assert.ifError(err)) etc., but not much beyond that (so no wrong use of `mustCall´). Is that what you mean?

addaleax · 2020-09-07T13:31:55Z

@tniessen Yeah, any common.mustCall() where the function body contains assert.ifError(err)

tniessen · 2020-09-08T16:20:04Z

Yeah, any common.mustCall() where the function body contains assert.ifError(err)

@addaleax I added the rule and fixed the resulting linter problems. However, I restricted it to only apply if the function body begins with assert.ifError, because some tests are allowing some errors in the callback, and only rejecting certain errors, e.g., like this:

something(common.mustCall((err) => {
  if (err && err.code === 'ERR_SOMETHING') {
    // Do something
    return;
  }
  assert.ifError(err);
});

tniessen · 2020-09-30T19:26:18Z

This has been open for more than three weeks and has neither been approved nor rejected nor otherwise reviewed. Ping @nodejs/testing (hopefully that's a good choice?).

Trott · 2020-09-30T19:52:37Z

This has been open for more than three weeks and has neither been approved nor rejected nor otherwise reviewed. Ping @nodejs/testing (hopefully that's a good choice?).

I go back and forth between thinking it's a good idea and might help catch some bugs in our tests...then thinking it may not be worth the churn/additional API...and then thinking that it doesn't matter that much either way because we can do whatever we want with the tests and then change it if we don't like it after a month or whatever.

Let's give this another 3 days or so and if no one else either approves or objects, I'll approve it and you can rebase and land. I want to give people with stronger opinions (and who are putting more thought into it) a chance to weigh in. But I think I like this change.

Replaced the following pattern with "mustSucceed()": mustCall$[ \n]*(\(erro?r?$[ \n]*=>[ \n]*\{?|function[ \n]*$erro?r?$[ \n]*\{)?[ \n]*assert\.ifError($erro?r?$)?;?[ \n]*\}?[ \n]*\)

Replaced the following pattern with "mustSucceed(($2)$3=>$4{\n": mustCall$[\n ]*\(erro?r?(,[\n ]*([^$]*))?\)([\n ]*)=>([\n ]*)\{[\n ]+assert\.ifError$erro?r?$;\n

Replaced the following pattern with "mustSucceed(function($2)$3{\n": mustCall$[\n ]*function[ \n]*\(erro?r?(, *([^$]*))?\)([ \t\n]*)\{[\n ]+assert\.ifError$erro?r?$;\n

Convert statements of the form assert(!err) to mustSucceed.

tniessen · 2020-10-13T15:53:08Z

@Trott It's been another two weeks, I will rebase this, and you (or anyone else) can either approve or close this.

nodejs-github-bot · 2020-10-13T17:13:05Z

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

nodejs-github-bot · 2020-10-14T10:16:22Z

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

nodejs-github-bot · 2020-10-14T18:53:56Z

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

tniessen · 2020-10-16T12:25:56Z

Ping @Trott. Sorry for pinging you again, I just don't want this to be forgotten for another few weeks and require more rebases. Feel free to ignore or close if you don't think approving it is the right thing to do.

ruyadorno · 2020-10-16T15:37:11Z

This is awesome @tniessen! Love it that you provided the eslint rules and the PR even catchs a bunch of scenarios of mustCall() being used without asserting the error.

I'm a big proponent of less abstractions in tests but all things weighted this is def one worth having 👍

github-actions · 2020-10-16T17:24:51Z

Commit Queue failed

- Loading data for nodejs/node/pull/35086 ✔ Done loading data for nodejs/node/pull/35086 ----------------------------------- PR info ------------------------------------ Title test: add common.mustSucceed (#35086) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch tniessen:add-mustsucceed -> nodejs:master Labels author ready, test Commits 9 - test: add common.mustSucceed - test: convert to mustSucceed - test: convert to mustSucceed - test: convert to mustSucceed - test: convert to mustSucceed - test: apply manual formatting to previous commits - test: use mustSucceed instead of mustCall - doc,test: document common.mustSucceed - tools,test: add linter rule for mustSucceed Committers 1 - Tobias Nießen PR-URL: https://github.com/nodejs/node/pull/35086 Reviewed-By: Ruy Adorno ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/35086 Reviewed-By: Ruy Adorno -------------------------------------------------------------------------------- ✔ Last GitHub Actions successful ℹ Last Full PR CI on 2020-10-14T18:53:56Z: https://ci.nodejs.org/job/node-test-pull-request/33623/ - Querying data for job/node-test-pull-request/33623/ ✔ Build data downloaded ✔ Last Jenkins CI successful ℹ This PR was created on Mon, 07 Sep 2020 01:32:28 GMT ✔ Approvals: 1 ✔ - Ruy Adorno (@ruyadorno): https://github.com/nodejs/node/pull/35086#pullrequestreview-510585252 -------------------------------------------------------------------------------- ✔ No git cherry-pick in progress ✔ No git am in progress ✔ No git rebase in progress -------------------------------------------------------------------------------- - Bringing origin/master up to date... From https://github.com/nodejs/node * branch master -> FETCH_HEAD ✔ origin/master is now up-to-date - Downloading patch for 35086 From https://github.com/nodejs/node * branch refs/pull/35086/merge -> FETCH_HEAD ✔ Fetched commits as 7f25fe8b67ee..e43195b5cbd1 -------------------------------------------------------------------------------- Auto-merging test/common/index.js [master c9f901e678] test: add common.mustSucceed Author: Tobias Nießen Date: Sun Sep 6 22:27:07 2020 +0200 1 file changed, 9 insertions(+) [master 49e28d63f3] test: convert to mustSucceed Author: Tobias Nießen Date: Sun Sep 6 23:39:24 2020 +0200 15 files changed, 23 insertions(+), 60 deletions(-) Auto-merging test/parallel/test-fs-rmdir-recursive.js Auto-merging test/parallel/test-dns-resolveany.js [master 8c7dfc7bb5] test: convert to mustSucceed Author: Tobias Nießen Date: Mon Sep 7 00:27:11 2020 +0200 77 files changed, 193 insertions(+), 387 deletions(-) [master 8148783856] test: convert to mustSucceed Author: Tobias Nießen Date: Mon Sep 7 01:13:37 2020 +0200 32 files changed, 64 insertions(+), 129 deletions(-) [master cf6c14ea88] test: convert to mustSucceed Author: Tobias Nießen Date: Mon Sep 7 01:22:49 2020 +0200 8 files changed, 28 insertions(+), 54 deletions(-) Auto-merging test/parallel/test-fs-rmdir-recursive.js [master 6bd8dd3ce2] test: apply manual formatting to previous commits Author: Tobias Nießen Date: Mon Sep 7 02:24:09 2020 +0200 43 files changed, 58 insertions(+), 116 deletions(-) [master 1058c28ee6] test: use mustSucceed instead of mustCall Author: Tobias Nießen Date: Mon Sep 7 02:07:13 2020 +0200 13 files changed, 40 insertions(+), 41 deletions(-) [master 826bc32c7b] doc,test: document common.mustSucceed Author: Tobias Nießen Date: Mon Sep 7 11:56:00 2020 +0200 2 files changed, 14 insertions(+) [master b4ad75875a] tools,test: add linter rule for mustSucceed Author: Tobias Nießen Date: Tue Sep 8 18:16:02 2020 +0200 20 files changed, 161 insertions(+), 97 deletions(-) create mode 100644 test/parallel/test-eslint-prefer-common-mustsucceed.js create mode 100644 tools/eslint-rules/prefer-common-mustsucceed.js ✔ Patches applied There are 9 commits in the PR. Attempting autorebase. Rebasing (2/18)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test: add common.mustSucceed

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

[detached HEAD f37312060c] test: add common.mustSucceed
Author: Tobias Nießen tniessen@tnie.de
Date: Sun Sep 6 22:27:07 2020 +0200
1 file changed, 9 insertions(+)
Rebasing (3/18)
Rebasing (4/18)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test: convert to mustSucceed

Replaced the following pattern with "mustSucceed()":

mustCall([ \n]((erro?r?)[ \n]=>[ \n]{?|function[ \n](erro?r?)[ \n]{)?[ \n]assert.ifError((erro?r?))?;?[ \n]}?[ \n])

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

[detached HEAD d602181ef6] test: convert to mustSucceed
Author: Tobias Nießen tniessen@tnie.de
Date: Sun Sep 6 23:39:24 2020 +0200
15 files changed, 23 insertions(+), 60 deletions(-)
Rebasing (5/18)
Rebasing (6/18)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test: convert to mustSucceed

Replaced the following pattern with "mustSucceed(($2)$3=>$4{\n":

mustCall([\n ](erro?r?(,[\n ]([^\)]))?)([\n ])=>([\n ]*){[\n ]+assert.ifError(erro?r?);\n

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

[detached HEAD 7da55c4db4] test: convert to mustSucceed
Author: Tobias Nießen tniessen@tnie.de
Date: Mon Sep 7 00:27:11 2020 +0200
77 files changed, 193 insertions(+), 387 deletions(-)
Rebasing (7/18)
Rebasing (8/18)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test: convert to mustSucceed

Replaced the following pattern with "mustSucceed(function($2)$3{\n":

mustCall([\n ]function[ \n](erro?r?(, ([^\)]))?)([ \t\n]*){[\n ]+assert.ifError(erro?r?);\n

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

[detached HEAD 18b1cca9eb] test: convert to mustSucceed
Author: Tobias Nießen tniessen@tnie.de
Date: Mon Sep 7 01:13:37 2020 +0200
32 files changed, 64 insertions(+), 129 deletions(-)
Rebasing (9/18)
Rebasing (10/18)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test: convert to mustSucceed

Convert statements of the form assert(!err) to mustSucceed.

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

[detached HEAD 021c76c096] test: convert to mustSucceed
Author: Tobias Nießen tniessen@tnie.de
Date: Mon Sep 7 01:22:49 2020 +0200
8 files changed, 28 insertions(+), 54 deletions(-)
Rebasing (11/18)
Rebasing (12/18)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test: apply manual formatting to previous commits

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

[detached HEAD f666cba22e] test: apply manual formatting to previous commits
Author: Tobias Nießen tniessen@tnie.de
Date: Mon Sep 7 02:24:09 2020 +0200
43 files changed, 58 insertions(+), 116 deletions(-)
Rebasing (13/18)
Rebasing (14/18)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
test: use mustSucceed instead of mustCall

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

[detached HEAD 6dd01218b0] test: use mustSucceed instead of mustCall
Author: Tobias Nießen tniessen@tnie.de
Date: Mon Sep 7 02:07:13 2020 +0200
13 files changed, 40 insertions(+), 41 deletions(-)
Rebasing (15/18)
Rebasing (16/18)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
doc,test: document common.mustSucceed

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

[detached HEAD be1aef8738] doc,test: document common.mustSucceed
Author: Tobias Nießen tniessen@tnie.de
Date: Mon Sep 7 11:56:00 2020 +0200
2 files changed, 14 insertions(+)
Rebasing (17/18)
Rebasing (18/18)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
tools,test: add linter rule for mustSucceed

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

[detached HEAD 51aabe80be] tools,test: add linter rule for mustSucceed
Author: Tobias Nießen tniessen@tnie.de
Date: Tue Sep 8 18:16:02 2020 +0200
20 files changed, 161 insertions(+), 97 deletions(-)
create mode 100644 test/parallel/test-eslint-prefer-common-mustsucceed.js
create mode 100644 tools/eslint-rules/prefer-common-mustsucceed.js

Successfully rebased and updated refs/heads/master.
✔ f37312060cfcf9753540430a5090667d98dd52c2
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 1:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length
✖ d602181ef613042d71a7ee171f182ab7ff702cc4
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✖ 3:72 Line should be <= 72 columns. line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 5:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length
✖ 7da55c4db43541ec30e118293c635642627f305b
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✖ 3:72 Line should be <= 72 columns. line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 5:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length
✖ 18b1cca9eb4671a95efdcd422ba47b8d6fe2dfc8
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✖ 3:72 Line should be <= 72 columns. line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 5:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length
✔ 021c76c096dd56c32a081fcd7bce884bdab5079d
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 3:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length
✔ f666cba22eba4a8289327dc7537a12a2ffa08ac7
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 1:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length
✔ 6dd01218b027710938123ff87f836e4632066a14
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 1:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length
✔ be1aef8738d7e2eff24216a386a943e870eb75bb
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 1:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length
✔ 51aabe80be548ac93afba9608b5d2ed475d9da4f
✔ 0:0 skipping fixes-url fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 1:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✔ 0:0 valid subsystems subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length
ℹ Please fix the commit message and try again.

Commit Queue action: https://github.com/nodejs/node/actions/runs/311127512

PR-URL: #35086 Reviewed-By: Ruy Adorno <ruyadorno@github.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

aduh95 · 2020-10-16T22:48:45Z

Landed in 30fb4a0

tniessen · 2020-10-16T23:09:55Z

Thank you for reviewing @ruyadorno and @Trott, and thank you for landing @aduh95.

PR-URL: #35086 Reviewed-By: Ruy Adorno <ruyadorno@github.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

tniessen added the test Issues and PRs related to the tests. label Sep 7, 2020

nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Sep 7, 2020

tniessen removed doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels Sep 7, 2020

Trott previously requested changes Sep 7, 2020

View reviewed changes

tniessen added the review wanted PRs that need reviews. label Sep 14, 2020

tniessen added 8 commits October 13, 2020 17:45

test: add common.mustSucceed

ce9389a

test: convert to mustSucceed

9c54c6b

Replaced the following pattern with "mustSucceed()": mustCall$[ \n]*(\(erro?r?$[ \n]*=>[ \n]*\{?|function[ \n]*$erro?r?$[ \n]*\{)?[ \n]*assert\.ifError($erro?r?$)?;?[ \n]*\}?[ \n]*\)

test: convert to mustSucceed

97b15e9

Replaced the following pattern with "mustSucceed(($2)$3=>$4{\n": mustCall$[\n ]*\(erro?r?(,[\n ]*([^$]*))?\)([\n ]*)=>([\n ]*)\{[\n ]+assert\.ifError$erro?r?$;\n

test: convert to mustSucceed

4e7fbd8

Replaced the following pattern with "mustSucceed(function($2)$3{\n": mustCall$[\n ]*function[ \n]*\(erro?r?(, *([^$]*))?\)([ \t\n]*)\{[\n ]+assert\.ifError$erro?r?$;\n

test: convert to mustSucceed

ea01060

Convert statements of the form assert(!err) to mustSucceed.

test: apply manual formatting to previous commits

4b8676e

test: use mustSucceed instead of mustCall

476d930

doc,test: document common.mustSucceed

c275bf0

tniessen force-pushed the add-mustsucceed branch from df0224e to d5c3a36 Compare October 13, 2020 16:59

tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 13, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 13, 2020

tools,test: add linter rule for mustSucceed

e43195b

tniessen force-pushed the add-mustsucceed branch from d5c3a36 to e43195b Compare October 14, 2020 09:58

tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 14, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 14, 2020

ruyadorno approved these changes Oct 16, 2020

View reviewed changes

tniessen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. and removed review wanted PRs that need reviews. labels Oct 16, 2020

github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 16, 2020

github-actions bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Oct 16, 2020

Trott approved these changes Oct 16, 2020

View reviewed changes

aduh95 pushed a commit that referenced this pull request Oct 16, 2020

test: add common.mustSucceed

30fb4a0

PR-URL: #35086 Reviewed-By: Ruy Adorno <ruyadorno@github.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

aduh95 closed this Oct 16, 2020

tniessen removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Oct 22, 2020

BethGriggs pushed a commit that referenced this pull request Dec 8, 2020

test: add common.mustSucceed

70cb708

PR-URL: #35086 Reviewed-By: Ruy Adorno <ruyadorno@github.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

BethGriggs mentioned this pull request Dec 10, 2020

v14.15.2 proposal #36476

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add common.mustSucceed #35086

test: add common.mustSucceed #35086

tniessen commented Sep 7, 2020

Trott commented Sep 7, 2020 •

edited

Trott commented Sep 7, 2020 •

edited

Trott left a comment

lpinca commented Sep 7, 2020 •

edited

tniessen commented Sep 7, 2020

tniessen commented Sep 7, 2020

addaleax commented Sep 7, 2020

tniessen commented Sep 7, 2020

addaleax commented Sep 7, 2020

tniessen commented Sep 8, 2020

tniessen commented Sep 30, 2020

Trott commented Sep 30, 2020

tniessen commented Oct 13, 2020 •

edited

nodejs-github-bot commented Oct 13, 2020

nodejs-github-bot commented Oct 14, 2020

nodejs-github-bot commented Oct 14, 2020

tniessen commented Oct 16, 2020

ruyadorno commented Oct 16, 2020

github-actions bot commented Oct 16, 2020

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

aduh95 commented Oct 16, 2020

tniessen commented Oct 16, 2020

test: add common.mustSucceed #35086

test: add common.mustSucceed #35086

Conversation

tniessen commented Sep 7, 2020

Checklist

Trott commented Sep 7, 2020 • edited

Trott commented Sep 7, 2020 • edited

Trott left a comment

Choose a reason for hiding this comment

lpinca commented Sep 7, 2020 • edited

tniessen commented Sep 7, 2020

tniessen commented Sep 7, 2020

addaleax commented Sep 7, 2020

tniessen commented Sep 7, 2020

addaleax commented Sep 7, 2020

tniessen commented Sep 8, 2020

tniessen commented Sep 30, 2020

Trott commented Sep 30, 2020

tniessen commented Oct 13, 2020 • edited

nodejs-github-bot commented Oct 13, 2020

nodejs-github-bot commented Oct 14, 2020

nodejs-github-bot commented Oct 14, 2020

tniessen commented Oct 16, 2020

ruyadorno commented Oct 16, 2020

github-actions bot commented Oct 16, 2020

PR-URL: #35086 Reviewed-By: Ruy Adorno ruyadorno@github.com

PR-URL: #35086 Reviewed-By: Ruy Adorno ruyadorno@github.com

PR-URL: #35086 Reviewed-By: Ruy Adorno ruyadorno@github.com

PR-URL: #35086 Reviewed-By: Ruy Adorno ruyadorno@github.com

PR-URL: #35086 Reviewed-By: Ruy Adorno ruyadorno@github.com

PR-URL: #35086 Reviewed-By: Ruy Adorno ruyadorno@github.com

PR-URL: #35086 Reviewed-By: Ruy Adorno ruyadorno@github.com

PR-URL: #35086 Reviewed-By: Ruy Adorno ruyadorno@github.com

PR-URL: #35086 Reviewed-By: Ruy Adorno ruyadorno@github.com

aduh95 commented Oct 16, 2020

tniessen commented Oct 16, 2020

Trott commented Sep 7, 2020 •

edited

Trott commented Sep 7, 2020 •

edited

lpinca commented Sep 7, 2020 •

edited

tniessen commented Oct 13, 2020 •

edited

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com

PR-URL: #35086
Reviewed-By: Ruy Adorno ruyadorno@github.com