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

Watch mode test pattern is global but it isn't quite obvious #2978

Closed
gaearon opened this issue Feb 22, 2017 · 23 comments
Closed

Watch mode test pattern is global but it isn't quite obvious #2978

gaearon opened this issue Feb 22, 2017 · 23 comments
Milestone

Comments

@gaearon
Copy link
Contributor

gaearon commented Feb 22, 2017

I run p to choose a file:

screen shot 2017-02-22 at 6 21 07 pm

Then I wanted to filter to a single test with t:

screen shot 2017-02-22 at 6 21 53 pm

From this screen, it seems like it would only run this test. But nope, it seemingly runs the match on all files:

screen shot 2017-02-22 at 6 22 41 pm

I'm not sure if this is intentional behavior or not, but it makes test pattern mode much less useful to me because I can never guess how many tests in other files may happen to share the same substring in their name. It's also never useful to me to filter all tests in the codebase by a word. I'd rather have take the exact match I chose with arrows than using the pattern I entered.

@rogeliog
Copy link
Contributor

rogeliog commented Feb 23, 2017

Currently, it only shows the name of the tests that are already cached. I agree that it makes the "t" pattern mode unpredictable... I wonder if we could add some messaging around it to make it less ambiguous 😄

@gaearon
Copy link
Contributor Author

gaearon commented Feb 23, 2017

Is it ever useful to run all tests matching a name globally?

I understand why it's convenient to search tests globally by their descriptions, but once I've found the one I need, I just want to select that test alone and focus on it. (As an alternative to writing fit.)

Fuzzy search for files make sense (e.g. if you use common prefixes or want to run all tests in a directory). But test descriptions are usually human readable so patterns don't work that well on them. Why not just treat Enter as selecting a test rather than running all tests that match entered pattern?

@cpojer
Copy link
Member

cpojer commented Feb 23, 2017

I agree with @gaearon but I preferred to get this feature out so we can collect ideas on how to optimize it in future releases. There are two questions we need to answer here:

  • When a pattern is entered to match files, should it stay applied when filtering for files and should we only show tests that match those files? How should we show this in the UI and should there be an escape hatch? (Like "press to clear the pattern"?).
  • Fuzzy search for tests instead of regex: we lose the consistency of the two typeaheads (both expect a regex) but it may still result in a much better user experience. At least we should make it work so that when you type a space, it converts it into \s behind the scenes.

cc @zouxuoz

@zouxuoz
Copy link
Contributor

zouxuoz commented Feb 23, 2017

I agree with @gaearon. @cpojer we missed really useful UX case and should resolve it. I think we can resolve it something like this:

  • not reset testFilePattern when user set testNamePattern;
  • reset testNamePattern when user set testFilePattern or run all test or run test for only changed files;
  • think about new feature with using the arrows for choosing current test;

I can send PR for resolve first and second points.

@rogeliog
Copy link
Contributor

I like that idea, it sounds really interesting

@rogeliog
Copy link
Contributor

I agree that the testNamePattern still needs more exploration to find the best UX

@gaearon
Copy link
Contributor Author

gaearon commented Apr 4, 2017

Is there a way to disable this feature until it is more solid? We'd like to ship Jest 19 in the next react-scripts update but IMO this will be confusing and users will just remember not to use it, so it will be harder to get them to discover it again when it's fixed.

@gaearon
Copy link
Contributor Author

gaearon commented Apr 4, 2017

(Another reason I'd like to disable it is because it can crash randomly: #2979)

@gaearon
Copy link
Contributor Author

gaearon commented Apr 4, 2017

The reason I'm being a little bit obnoxious here is because I care deeply about the quality of Jest integration. Jest has had very weird issues in the past, and it took a lot of effort from @cpojer and @DmitriiAbramov to get it to the point where it’s good enough to be bundled by default in CRA. I’d like to make sure that we don’t drop that quality bar over time now that Jest is bundled. Unfortunately I don’t have time to work on this myself. 😞

@thymikee
Copy link
Collaborator

thymikee commented Apr 4, 2017

What about hiding it form Watch Usage prompt until we make it work flawlessly? It would still be there but just hidden, because it's like still experimental feature?

@gaearon
Copy link
Contributor Author

gaearon commented Apr 4, 2017

Yes, that would be perfect for us!

@gaearon
Copy link
Contributor Author

gaearon commented Apr 4, 2017

(I guess you could still trigger it accidentally but I'm fine with that)

@thymikee
Copy link
Collaborator

thymikee commented Apr 4, 2017

Yeah, but it still tells you how to exit :D

@thymikee
Copy link
Collaborator

thymikee commented Apr 4, 2017

Or, maybe it makes sense to change the wording?

 › Press t to filter by a test name regex pattern (experimental).

@gaearon
Copy link
Contributor Author

gaearon commented Apr 4, 2017

I don't know, up to you. I'd probably just hide it while it's confusing since it takes screen space.

@thymikee
Copy link
Collaborator

thymikee commented Apr 4, 2017

PR addressed. btw, with next release space won't be a big deal

screen shot 2017-04-04 at 22 27 37

@gaearon
Copy link
Contributor Author

gaearon commented Apr 4, 2017

Oh, that's nice. 👍

@TobiasBales
Copy link

as of #3253 is this good to go for now?

@cpojer
Copy link
Member

cpojer commented Apr 11, 2017

We should bring back this mode and fix it before actually releasing Jest 20 (which is still a little while off, it seems :) ).

@TobiasBales
Copy link

Will this be released as 19.x though?
Or is it considered a breaking change and will not be released until it's done "properly".

Asking for integration of jest 19 into react-create-app

@cpojer
Copy link
Member

cpojer commented Apr 11, 2017

We are probably not going to make another release to Jest 19.

@thymikee thymikee modified the milestone: Jest 20 Apr 21, 2017
@cpojer
Copy link
Member

cpojer commented Apr 27, 2017

This will be fixed in Jest 20.

@cpojer cpojer closed this as completed Apr 27, 2017
@Timer
Copy link

Timer commented Apr 27, 2017

Any timeline on Jest 20? 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants