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.

Sign up for GitHub

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
Merged

Fix: Prevent maintaining RegExp state between multiple tests #9289

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
08ca5b2
fix: prevent maintaining RegExp state between calls
wsmd Dec 10, 2019
88a8cbd
add changelog entry
wsmd Dec 10, 2019
0312bd2
Merge branch 'master' into fix-regexp-global
wsmd Jan 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions packages/expect/src/__tests__/matchers.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1615,6 +1615,13 @@ describe('.toMatch()', () => {
it('escapes strings properly', () => {
jestExpect('this?: throws').toMatch('this?: throws');
});

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 •
edited
Loading

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 •
edited
Loading

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

Copy link

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

jestExpect(regex.lastIndex).toBe(0);
});
});

describe('.toHaveLength', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/expect/src/matchers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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!


const message = pass
? () =>
Expand Down