Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

util: improve inspects RegExp support #25192

Closed
wants to merge 2 commits into from

Conversation

BridgeAR
Copy link
Member

Please check the commit messages as description.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Dec 23, 2018
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.
@BridgeAR
Copy link
Member Author

@Trott
Copy link
Member

Trott commented Dec 23, 2018

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/ ✔️

const expectedWithoutProto = `[${base.name}: null prototype] ${rawExpected}`;
assert.strictEqual(util.inspect(value), expected);
value.foo = 'bar';
assert.notStrictEqual(util.inspect(value), expected);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, it is sufficient as you said above but thought it could be better using strictEqual. Anyways this is not a blocker to land.

Copy link
Contributor

@antsmartian antsmartian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@antsmartian antsmartian added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 24, 2018
@BridgeAR
Copy link
Member Author

@nodejs/util PTAL.

This needs another review.

BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 27, 2018
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Dec 27, 2018
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR
Copy link
Member Author

Landed in bd13afb and c9d08c7

@BridgeAR BridgeAR closed this Dec 27, 2018
targos pushed a commit that referenced this pull request Jan 1, 2019
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jan 1, 2019
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
@BridgeAR BridgeAR deleted the improve-reg-exp-support branch January 20, 2020 11:48
@BridgeAR BridgeAR restored the improve-reg-exp-support branch January 20, 2020 11:49
@BridgeAR BridgeAR deleted the improve-reg-exp-support branch January 20, 2020 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants