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

Fix: Prevent maintaining RegExp state between multiple tests #9289

Merged
merged 3 commits into from
Jan 19, 2020

Conversation

wsmd
Copy link
Contributor

@wsmd wsmd commented Dec 10, 2019

Summary

Fixes #9283

Currently expect maintains the state of RegExp objects when using toMatch(RegExp) or toThrow(RegExp) with a global flag g.

For example, the second assertion here fails:

const regex = /[f]\d+/g;
expect('f123').toMatch(regex);
expect('f456').toMatch(regex); // Fails!

What's expected there is for both assertions to pass.

This happens because expect is using the same RegExp object for every test. This can be solved by instantiating a new RegExp object for every test.

Test plan

Added a new test.

@@ -835,7 +835,7 @@ const matchers: MatchersObject = {
const pass =
typeof expected === 'string'
? received.includes(expected)
: expected.test(received);
: new RegExp(expected).test(received);
Copy link
Contributor Author

@wsmd wsmd Dec 10, 2019

Choose a reason for hiding this comment

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

I doubt there be will any issues with instantiating a new RegExp object for every call, but if this is a concern, I could limit that to only RegExps with the global/sticky flag since that's where this issue seems most likely to arise. Let me know what you think!

@wsmd wsmd force-pushed the fix-regexp-global branch 2 times, most recently from 78f3208 to 14fda54 Compare December 10, 2019 02:41
it('does not maintain RegExp state between calls', () => {
const regex = /[f]\d+/gi;
jestExpect('f123').toMatch(regex);
jestExpect('F456').toMatch(regex);
Copy link
Collaborator

@thymikee thymikee Dec 10, 2019

Choose a reason for hiding this comment

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

I don't really know how I feel about this. As a JS user, I would expect this regex to be stateful and would be surprised that it's re-set by a specific matcher.
Not to mention this is a breaking change and needs documentation.
cc @pedrottimark @SimenB

Copy link
Contributor Author

@wsmd wsmd Dec 10, 2019

Choose a reason for hiding this comment

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

I agree @thymikee, FWIW that's the entire point of having a global flag in the first place is to be able to test against multiple matches. The issue reported via #9283 could be solved by removing the global from the RegExp since it's doing what it's supposed to do.

That said, #9283 was marked as a bug because this (specifically with Jest) could also be considered a regression since this was not the intended behavior before v24 was released.

I'm not sure if this behavior change was intentional or documented somewhere (since it was also a breaking change), or if it was just a regression that needs to be fixed (hence this PR).

Choose a reason for hiding this comment

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

For what it's worth, I wouldn't expect Jest to change the state of an object, including a regular expression. It seems like it creates confusion and potential for false-positives in cases where the regular expression is being tested against several strings in the same test. If I want to test the lastIndex of a regex, I think I would test that directly:

regex.test('f123');
expect(regex.lastIndex).toBe(4);
regex.test('f456');
expect(regex.lastIndex).toBe(0);

Copy link
Contributor

Choose a reason for hiding this comment

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

You have my apology that I missed commenting on the issue or pull request before this.

It was my mistake in #8008 not to keep the code path to create a new instance for expected RegExp when I changed an expected string to match using includes method.

Therefore, this PR fixes what I broke, no more and no less.

@wsmd Thanks for your work. If you can merge changes from master and resolve the conflict in CHANGELOG.md then this is ready to merge.

There is a related (but separate) documentation chore in ExpectAPI.md under toMatch to adapt the example in #9283 and in the preceding #9289 (comment) so people understand how to write tests that are independent of RegExp state or intentionally testing RegExp state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @pedrottimark! The merge conflict is now resolved. 👍

@codecov-io
Copy link

Codecov Report

Merging #9289 into master will decrease coverage by 0.18%.
The diff coverage is 46.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9289      +/-   ##
==========================================
- Coverage   64.98%   64.79%   -0.19%     
==========================================
  Files         278      281       +3     
  Lines       11882    12009     +127     
  Branches     2926     2962      +36     
==========================================
+ Hits         7721     7781      +60     
- Misses       3532     3597      +65     
- Partials      629      631       +2
Impacted Files Coverage Δ
e2e/v8-coverage/no-sourcemap/x.css 100% <ø> (ø)
packages/jest-config/src/ValidConfig.ts 100% <ø> (ø) ⬆️
packages/jest-config/src/Defaults.ts 100% <ø> (ø) ⬆️
packages/jest-config/src/normalize.ts 77.74% <ø> (ø) ⬆️
...ckages/jest-reporters/src/generateEmptyCoverage.ts 66.66% <0%> (-22.23%) ⬇️
packages/jest-config/src/index.ts 11.76% <0%> (-0.18%) ⬇️
...ackages/jest-jasmine2/src/assertionErrorMessage.ts 0% <0%> (ø) ⬆️
packages/jest-runner/src/runTest.ts 2.19% <0%> (-0.25%) ⬇️
packages/jest-circus/src/formatNodeAssertErrors.ts 11.86% <0%> (-1.1%) ⬇️
packages/jest-snapshot/src/State.ts 0% <0%> (ø) ⬆️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88a8cbd...0312bd2. Read the comment docs.

Copy link
Contributor

@pedrottimark pedrottimark left a comment

Choose a reason for hiding this comment

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

By the way, I verified locally that new RegExp(regexp) copies any flags like i

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

toMatch behaves oddly for regex with global flag
6 participants