util: improve inspects RegExp support - #25192
Conversation
Sorry, something went wrong.
|
@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2048/pipeline |
Sorry, something went wrong.
This adds support for inspect to distinguish regular expression subclasses and ones with null prototype from "normal" regular expressions.
So far we do not test all data types for subclasses and this extends the existing tests for WeakSet, WeakMap and BigInt64Array.
Sorry, something went wrong.
|
AIX failure is a known issue (#24305). I'll open a PR to mark it as flaky. Pi issue is a new one. It's a crash rather than a simpler test failure and it could be something unrelated to the test itself. Will open an issue for it to track if it recurs. Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/19769/ ✔️ |
Sorry, something went wrong.
| const expectedWithoutProto = `[${base.name}: null prototype] ${rawExpected}`; | ||
| assert.strictEqual(util.inspect(value), expected); | ||
| value.foo = 'bar'; | ||
| assert.notStrictEqual(util.inspect(value), expected); |
There was a problem hiding this comment.
One small suggestion: may be we want to pull out the test cases for properties in a separate block. Would be happy to see strictEqual rather than notStrictEqual(as it can silently fail on wrong op's).
Sorry, something went wrong.
There was a problem hiding this comment.
We have lots of tests which verify that the properties are actually displayed correct and they always hit the same code path. Therefore verifying that it's a different result should be sufficient.
Below there's also another test to verify that at least the property is indeed displayed as expected.
The reason for not using the strict comparison here is that each data type has different ways of displaying some parts and it's difficult to generalize that.
Sorry, something went wrong.
There was a problem hiding this comment.
Sure, it is sufficient as you said above but thought it could be better using strictEqual. Anyways this is not a blocker to land.
Sorry, something went wrong.
antsmartian
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
|
@nodejs/util PTAL. This needs another review. |
Sorry, something went wrong.
This adds support for inspect to distinguish regular expression subclasses and ones with null prototype from "normal" regular expressions. PR-URL: nodejs#25192 Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
So far we do not test all data types for subclasses and this extends the existing tests for WeakSet, WeakMap and BigInt64Array. PR-URL: nodejs#25192 Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Sorry, something went wrong.
This adds support for inspect to distinguish regular expression subclasses and ones with null prototype from "normal" regular expressions. PR-URL: #25192 Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
So far we do not test all data types for subclasses and this extends the existing tests for WeakSet, WeakMap and BigInt64Array. PR-URL: #25192 Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This adds support for inspect to distinguish regular expression subclasses and ones with null prototype from "normal" regular expressions. PR-URL: nodejs#25192 Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
So far we do not test all data types for subclasses and this extends the existing tests for WeakSet, WeakMap and BigInt64Array. PR-URL: nodejs#25192 Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Please check the commit messages as description.
Checklist