test,assert: remove internal errorCache property by Trott · Pull Request #23304 · nodejs/node

Trott · 2018-10-07T03:21:22Z

First commit:

test: remove internal errorCache property

The internal assert modules errorCache property is exposed only for
testing. The one test that used it is rewritten here to not use it.

This has the following advantages:

The test now makes sure that there is an empty cache in a more robust
way. Instead of relying on the internal implementation of
errorCache, it simply spawns a separate process.
One less test using the --expose-internals flag.

Second commit:

assert: remove internal errorCache property

The internal assert module exposed an errorCache property solely for
testing. It is no longer necessary. Remove it.

Checklist

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

The internal `assert` modules `errorCache` property is exposed only for testing. The one test that used it is rewritten here to not use it. This has the following advantages: * The test now makes sure that there is an empty cache in a more robust way. Instead of relying on the internal implementation of `errorCache`, it simply spawns a separate process. * One less test using the `--expose-internals` flag.

The internal assert module exposed an errorCache property solely for testing. It is no longer necessary. Remove it.

nodejs-github-bot · 2018-10-07T03:21:23Z

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

Trott · 2018-10-07T23:57:32Z

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

BridgeAR

This is definitely a good idea!

BridgeAR · 2018-10-08T12:50:47Z

test/parallel/test-assert-builtins-not-read-from-filesystem.js

+  assert.ok(threw);
+} else {
+  const { writeFileSync } = require('fs');
+  const [, , , filename, line, column] = process.argv;


I somehow fail to see how the line and column is passed through to the child.

Ugh! That's because it's not! Fixed! Thanks.

(See line 28. I had line, column missing before.)

Trott · 2018-10-08T14:31:29Z

Post-fixup CI: https://ci.nodejs.org/job/node-test-pull-request/17681/ ✔️

Trott · 2018-10-09T03:21:58Z

/ping @nodejs/assert @nodejs/testing This needs one more review.

The internal `assert` modules `errorCache` property is exposed only for testing. The one test that used it is rewritten here to not use it. This has the following advantages: * The test now makes sure that there is an empty cache in a more robust way. Instead of relying on the internal implementation of `errorCache`, it simply spawns a separate process. * One less test using the `--expose-internals` flag. PR-URL: nodejs#23304 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

The internal assert module exposed an errorCache property solely for testing. It is no longer necessary. Remove it. PR-URL: nodejs#23304 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Trott · 2018-10-09T13:43:07Z

Landed in 157d507...4d58c08

The internal `assert` modules `errorCache` property is exposed only for testing. The one test that used it is rewritten here to not use it. This has the following advantages: * The test now makes sure that there is an empty cache in a more robust way. Instead of relying on the internal implementation of `errorCache`, it simply spawns a separate process. * One less test using the `--expose-internals` flag. PR-URL: #23304 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

The internal assert module exposed an errorCache property solely for testing. It is no longer necessary. Remove it. PR-URL: #23304 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

The internal `assert` modules `errorCache` property is exposed only for testing. The one test that used it is rewritten here to not use it. This has the following advantages: * The test now makes sure that there is an empty cache in a more robust way. Instead of relying on the internal implementation of `errorCache`, it simply spawns a separate process. * One less test using the `--expose-internals` flag. PR-URL: nodejs#23304 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

The internal assert module exposed an errorCache property solely for testing. It is no longer necessary. Remove it. PR-URL: nodejs#23304 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

The internal `assert` modules `errorCache` property is exposed only for testing. The one test that used it is rewritten here to not use it. This has the following advantages: * The test now makes sure that there is an empty cache in a more robust way. Instead of relying on the internal implementation of `errorCache`, it simply spawns a separate process. * One less test using the `--expose-internals` flag. PR-URL: #23304 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

The internal assert module exposed an errorCache property solely for testing. It is no longer necessary. Remove it. PR-URL: #23304 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Trott added 2 commits October 6, 2018 18:28

assert: remove internal errorCache property

cd2e66a

The internal assert module exposed an errorCache property solely for testing. It is no longer necessary. Remove it.

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

BridgeAR reviewed Oct 8, 2018

View reviewed changes

fixup! assert: remove internal errorCache property

15993e5

Trott force-pushed the vancouver-03 branch from 45eb9ad to 15993e5 Compare October 8, 2018 14:29

BridgeAR approved these changes Oct 8, 2018

View reviewed changes

thefourtheye approved these changes Oct 9, 2018

View reviewed changes

Trott closed this Oct 9, 2018

targos mentioned this pull request Oct 10, 2018

Release proposal: v10.12.0 #23313

Merged

beevelop mentioned this pull request Oct 11, 2018

2018-10-10 Version 10.12.0 (Current) @targos beevelop/docker-nodejs#229

Closed

Trott deleted the vancouver-03 branch January 13, 2022 22:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test,assert: remove internal errorCache property#23304

test,assert: remove internal errorCache property#23304
Trott wants to merge 3 commits intonodejs:masterfrom
Trott:vancouver-03

Trott commented Oct 7, 2018

Uh oh!

nodejs-github-bot commented Oct 7, 2018

Uh oh!

Trott commented Oct 7, 2018

Uh oh!

BridgeAR left a comment

Uh oh!

BridgeAR Oct 8, 2018

Uh oh!

Trott Oct 8, 2018

Uh oh!

Trott Oct 8, 2018

Uh oh!

Trott commented Oct 8, 2018 •

edited

Loading

Uh oh!

Trott commented Oct 9, 2018

Uh oh!

Trott commented Oct 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

Trott commented Oct 7, 2018

First commit:

Second commit:

Checklist

Uh oh!

nodejs-github-bot commented Oct 7, 2018

Uh oh!

Trott commented Oct 7, 2018

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

Trott Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

Trott Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Trott commented Oct 9, 2018

Uh oh!

Trott commented Oct 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Trott commented Oct 8, 2018 •

edited

Loading