Jest: Breaking change in --testPathPattern

[butchyyyy]

Since Jest 22 the fix of https://github.com/facebook/jest/issues/3793 via https://github.com/facebook/jest/pull/5054 breaks all tooling that already passed escaped path separator in the argument (e.g. IntelliJ IDEA / WebStorm Jest test runner).

The documentation states "--testPathPattern=<regex>", so the previous behaviour was imho correct. The argument accepts regex and it's up to the user to correctly escape the string (including windows path separator).

If the change of behaviour was intentional, it should be at the very least mentioned in the version 22 release notes / migration guide from previous version.

[cpojer]

cc @seanpoulter

[seanpoulter]

Sorry to hear you're having issues @butchyyyy. I contributed #5054 to escape the patterns the same way as the "Watch Usage" prompt and to fix problems with Windows path separators. Like you've reported, I've confirmed there is a regression for the escaped Windows path separators. I'll take the blame on this one for not adding even more unit tests to confirm the changes were safe. My apologies.

I've gone through the path separator escaping on two systems to confirm the change in behavior. The output follows in this table:

I think the best course of action is a quick code fix, unit tests, and leaving the docs as is.

Any way I can help you get this out in the next patch @cpojer?

[seanpoulter]

@butchyyyy, are there other cases where the escaping fails?

[butchyyyy]

Sorry for the late reply @seanpoulter, it has been a busy day. Thanks for the fix, but unfortunately there's another case the escaping fails.

To provide you with an example, IDE builds an absolute escaped path to test file and sends that as --testPathPattern, e.g.:

--testPathPattern ^C:\\develop\\SuchCoolProject\\src\\__tests__\\component\\verify\\CoolComponent\.react\-test\.tsx$

Before #5230 that would end up as

Pattern: ^C:\\\\develop\\\\SuchCoolProject\\\\src\\\\__tests__\\\\component\\\\verify\\\\CoolComponent\.react\\-test\.tsx$ - 0 matches

After #5230 it will end up as

Pattern: ^C:\\develop\\SuchCoolProject\\src\\__tests__\\component\\verify\\CoolComponent\.react\\-test\.tsx$ - 0 matches

The hyphen gets double escaped while it should not. In fact testPathPattern will incorrectly double escape any escape sequence besides \\ and \. which are specifically handled (however unlikely they are to be used in the testPathPattern).

While i understand the goal of unifying the behaviour of --testPathPattern and --watch mode i kinda still think the best course of action would be to remove replacePathSepForRegex in the --testPathPattern processing and leave the escaping up to the tool/user invoking jest.

[seanpoulter]

Sorry for the late reply @seanpoulter, it has been a busy day.

Don't be sorry. I'm doing this in my "spare" time too.

The hyphen gets double escaped while it should not.

I'm a bit confused. Why is the hyphen escaped? I didn't think it was a special character in JavaScript regular expressions. Here's the MDN RexExp docs for Special characters meaning in regular expressions.

i kinda still think the best course of action would be to remove replacePathSepForRegex in the --testPathPattern processing and leave the escaping up to the tool/user invoking jest.

My motivation for contributing the PRs was to provide cross-platform support for projects that have a --testPathPattern set in package.json. When you're developing on more than one OS, it's helpful for Jest to support either path separator.

[butchyyyy]

I was trying to wrap my head around that too. There are some rare cases where it makes sense to escape hyphen, for example if you would want to match "A" "-" or "Z" character you could write [A\-Z] and there the hyphen escape makes sense (otherwise you would be matching everything from A to Z).

I cannot tell you how exactly Idea escapes the path since the jest test runner is part of Ultimate edition (only Community edition is open source). Wild guess would be they escape any character that can have special meaning in regular expression.

I don't want to be a nuissance so i can live with working around the problem by removing hyphen from the test file names.

[seanpoulter]

Why don't we try and get someone from the IDEA team in the discussion? There doesn't seem to be any open issues yet. Do you have access to product support, or should I jump on the community forum?

[SimenB]

/cc @develar

[seanpoulter]

That's perfect, thanks @SimenB.

@develar, it seems I've broken Jest support in IJ with a PR released with Jest v22. I think the subsequent PR #5230 should fix the issues, but @butchyyyy's suggested there may still be problems. I'd like to make sure we're all good on the IJ end. Is there someone on the team who can take a look? I'm happy to discuss this on a call if you'd prefer -- send me an email to coordinate.

[butchyyyy]

There certainly still are issues. The subsequent PR just fixed the most common use cases. Many edge cases will fail becuase replacePathSepForRegex will mangle valid regexp passed into the --testPathPattern parameter.

Trying to fix the replacePathSepForRegex to handle more edge cases would turn the method into a mess and you still can't properly handle all of them. The idea of being able to correctly escape windows path separator in a regexp is fundamentally wrong. Let me give you an example:

consider following --testPathPattern c:\dev\sample\d\*\.js

Now is there "d" directory in the path or should the pattern match sample1...sample9 directories? You can't tell... only the user that defines the --testPathPattern regexp knows.

P.S.: I don't really see the point of dragging IJ into this unless you change the docs/behaviour. Then it would make sense to make a change request to make the IJ plugin compatible with Jest 22+. As of now, everything works as it should on their end. They pass in a valid regexp.

[seanpoulter]

Many edge cases will fail becuase replacePathSepForRegex will mangle valid regexp

The watch usage file pattern will continue to mangle regular expressions. :disappointed:

The idea of being able to correctly escape windows path separator in a regexp is fundamentally wrong.

Yes, there will always be cases when the \ is ambiguous. Can we improve cross-platform support and makes it friendlier for new Windows users despite this?

I'm game to revert replacePathSepForRegex and only swap swap POSIX path separators on Windows. That'd let us use / as a cross-platform path separator. We can update the docs accordingly to say "regexp preferring /".

If we don't escape Windows separators, how about displaying a warning to coach Windows users to use / or \\ when:

• your pattern uses \ only
• there are no matches
• the directory exists
  
  

I don't really see the point of dragging IJ into this

They'll get a heads up on the issue, can check if the - is properly escaped, may have time to contribute to the issue, and they can comment as they see fit.

[butchyyyy]

I'm game to revert replacePathSepForRegex and only swap swap POSIX path separators on Windows.

I like the idea, but changing that in the replacePathSepForRegex would affect the watch usage right? I can't assess how much of a problem that would be. Is it always manual input in console?

[seanpoulter]

changing that in the replacePathSepForRegex would affect the watch usage right?

Ugh, sorry @butchyyyy. I meant remove the call to replacePathSepForRegex in the CLI arg only. I'd leave the watch usage as is.

[seanpoulter]

I suggested displaying a warning when a testPathPattern uses the wrong path separator for the platform. How about displaying these when there are no matches, the path exists, and only the wrong separator is used?

For \\ on POSIX systems:

Pattern: packages\\jest-config\\ - 0 matches - replace "\\" with "/" to match existing path

For \ on Windows:

Pattern: packages\jest-config\ - 0 matches - replace "\" with "/" or "\\" in the regular expression to match existing path

[butchyyyy]

I meant remove the call to replacePathSepForRegex in the CLI arg only. I'd leave the watch usage as is.

Fine by me as that means going back to way it worked in Jest 21.

Also ok with Introducing / for \\ replacement in the --testPathPattern on Windows

[segrey]

@butchyyyy @seanpoulter Great work! Thanks for the heads up, I'm maintaining IntelliJ IDEA / WebStorm Jest test runner. Yes, it's was reported as https://youtrack.jetbrains.com/issue/WEB-30377. Fixed in 2017.3.3 by replacing path separators (\, /) with . (any char) on Windows. E.g. the following pattern will be passed for the above-mentioned example:
--testPathPattern ^C:.develop.SuchCoolProject.src.__tests__.component.verify.CoolComponent\.react-test\.tsx$

@seanpoulter You're right that there is no need to escape - as it's enough to escape [ and ] (https://github.com/lodash/lodash/blob/master/escapeRegExp.js). Thanks, will update IJ escaping logic.
Unfortunately, IJ/WebStorm Jest test runner source code is closed, but I'm here to assist if you have any questions regarding Jest integration.

[seanpoulter]

Thanks and sorry for the bug @segrey. Glad it'll be fixed in 2017.3.3.

When I get a spare hour I'll revert the change as discussed in my earlier comment and submit a PR to close the issue. I'll make a new issue for the enhancement so we can get this closed and deployed.

[segrey]

@seanpoulter Thanks, I like the proposed solution.