Use testPathPattern as a filter on top of findRelatedTests #8311

mackness · 2019-04-11T16:52:52Z

Summary

Based on this issue #5129 --findRelatedTests is overriding --testPathPattern. The goal of this PR is to apply --testPathPattern as a filter on top of results returned by --findRelatedTests.

Test plan

Planning on adding tests for the _getTestPaths method in SearchSource.ts

…g unit tests

jeysal · 2019-04-11T22:13:52Z

@mackness Thanks for taking time to tackle this, let us know if you get stuck anywhere :)

jeysal · 2019-04-11T22:32:54Z

packages/jest-core/src/SearchSource.ts

+      if (globalConfig.testPathPattern !== null) {
+        return {
+          tests: relatedTests.tests.filter(test =>
+            new RegExp(globalConfig.testPathPattern).test(test.path),


I'd probably try to reuse the logic in _filterTestPathsWithStats on the related tests, hopefully this helps as a hint

That is very helpful, I was wondering why was not seeing the stats output when 0 tests were matched. Making the update now!

We only want to _filterTestPathsWithStats if testPathPattern was explicitly passed as an argument. The issue is that it's not possible to simply null check globalConfig.testPathPattern to determine if it was explicity passed becase that field gets populated with values passed to findRelatedTests also. This approach compares the values in globalConfig.testPathPattern to the values in globalConfig.nonFlagArgs and if it is a 1 to 1 match it is assumed that testPathPattern was not explicity passed as an argument.

codecov-io · 2019-04-12T15:39:02Z

Codecov Report

Merging #8311 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #8311      +/-   ##
==========================================
- Coverage   62.17%   62.15%   -0.03%     
==========================================
  Files         266      266              
  Lines       10683    10687       +4     
  Branches     2597     2598       +1     
==========================================
  Hits         6642     6642              
- Misses       3452     3456       +4     
  Partials      589      589

Impacted Files	Coverage Δ
packages/jest-core/src/SearchSource.ts	`55.91% <0%> (-2.52%)`	⬇️

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 034ab25...d83b4c6. Read the comment docs.

jeysal · 2019-04-13T14:44:45Z

@mackness I just took a look at your updates - it appears that the root cause of what you worked around with d83b4c6 is that we are using all the non-option arguments to find related tests, so the non-option arguments become both parts of testPathPattern and findRelatedTest roots. I think the way forward is to let yargs parse findRelatedTests as an array instead of just a boolean flag (which seems like the right thing to do anyway), see https://github.com/facebook/jest/blob/538cef4c4ed15686a40380550041c3993a8e26ce/packages/jest-cli/src/cli/args.ts#L268
This will require adapting the config typings and then you should be able to use the config properties testPathPattern and findRelatedTests separately :)
cc @SimenB in case I'm missing something

mackness · 2019-04-13T15:32:28Z

@jeysal That seems like a much better approach, my workaround definitely did not feel right. I'll let you know if I runt into any issues with the implementation, thanks for the suggestion!

mackness · 2019-04-17T15:26:57Z

Hi @jeysal I am letting yargs parse findRelatedTests as a string array and a few issues came up:

A couple hard to solve typing issues. (1, 2)
For some reason when findRelatedTests is not explicitly passed the option is parsed as [undefined]. Maybe yargs parses it this way for some reason? workaround
Lots of failing snapshot tests, not sure which snapshots I can simply update and which ones are failing for legitimate reasons.

I will keep trying to track down the root cause of 1 and 2 but a little bit of direction with 3 would be greatly appreciated, thanks!

jeysal · 2019-04-18T10:38:19Z

@mackness After a quick glance, most of the snapshots seem like legimitate failures that need fixing to me.
For e.g. this kind of stuff

see https://github.com/facebook/jest/blob/3e05c06aac5778c4f3365b3c673a7ce68b6a392d/packages/jest-reporters/src/summary_reporter.ts#L192

There's also coverage related failures. Coverage has special interaction with stuff like findRelatedTests and onlyChanged. Looking at the places touched by #5601 and #6736 would probably help. In general, grepping for config.findRelatedTests to find locations impacted by the config change is a good idea I think :)

mackness · 2019-04-18T15:54:49Z

@jeysal Ok awesome I'll work on making the tests pass and adding a couple tests of my own, thanks!

findRelatedTests used to be a boolean field. It has been updated to be Array<string>. We will check the length of the array to determine if findRelatedTests is falsy or truthy since 0 will evaluate to false and aything greater than 0 will evaluate to true.

mackness · 2019-04-24T14:32:19Z

yep, I'll make the change :)

simple test that asserts --testPathPattern will filter tests matched by --findRelatedTests

for some reason this lint command works locally but fails in CI eslint . --cache --report-unused-disable-directives --ext js,jsx,ts,tsx,md --format junit -o reports/junit/js-lint-results.xml

jeysal · 2019-06-03T21:55:43Z

@mackness Let me know when you think this is ready for review again or if you run into any other problems :)

mackness · 2019-06-03T22:55:21Z

@jeysal Thanks! I am planning on adding a few more unit tests and there are a couple typing issues I need to take care of then It will be ready for another review.

One question this fails in CI but it does not fail locally. Also it does not seem related to code I changed. Do you know what might be causing eslint to complain about no-unresolved?

Maybe because fsevents is an optional dependency in jest-haste-map?

https://github.com/facebook/jest/blob/master/packages/jest-haste-map/package.json#L35

jeysal · 2019-06-04T21:53:54Z

Yeah I get those too, idk why - maybe @SimenB knows?
I usually just ignore those and pretend that lint passed if it's just those three errors :P

SimenB · 2019-06-04T22:15:46Z

I think this is due to fsevents only being available on mac, so if you're not on mac (like CI is) it errors

github-actions · 2022-09-08T18:57:03Z

This PR is stale because it has been open 1 year with no activity. Remove stale label or comment or this will be closed in 30 days.

github-actions · 2022-10-08T19:05:50Z

This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one.

github-actions · 2022-11-08T00:10:29Z

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.

use testPathPattern as a filter on top of findRelatedTests

92837e0

facebook-github-bot added the cla signed label Apr 11, 2019

null check testPathPattern and eventually get around to fixing failin…

808564a

…g unit tests

jeysal reviewed Apr 11, 2019

View reviewed changes

Mack Solomon added 3 commits April 11, 2019 18:12

use _filterTestPathsWithStats to to filter by with testPathPattern

f7eafee

remove unnecessary code

5eddcbd

Mack Solomon added 2 commits April 17, 2019 07:43

convert findRelatedTests parameter from a boolean to a string array

7ee3e54

fix linting issues

9d7aed1

mackness added 3 commits May 4, 2019 09:05

explicitly check for more than 0 related tests

b9ad8bc

explicit find related tests length check

2613dd0

fix failing normalize spec

17b4310

JoshRosenstein mentioned this pull request May 7, 2019

chore: types #8430

Closed

mackness added 8 commits June 2, 2019 11:46

normalize and null check find related tests

f95a0d4

Merge branch 'master' into find-related-tests-bug

bfa2569

lockfile

a85dd4b

udpate show config and log debug messages snapshots

4e99139

update find related files snapshots

4169622

udpate summary message in find related tests spec

9c001f7

add a simple --testPathPattern assertion

144e6f0

simple test that asserts --testPathPattern will filter tests matched by --findRelatedTests

disable prefer-const rule

e73ecd7

disable no-unresolved rule for fsevents

c33284d

for some reason this lint command works locally but fails in CI eslint . --cache --report-unused-disable-directives --ext js,jsx,ts,tsx,md --format junit -o reports/junit/js-lint-results.xml

mackness added 2 commits June 8, 2019 12:09

Merge branch 'master' into find-related-tests-bug

a938c25

remove unused @ts-ignore comments

7dc3269

jeysal mentioned this pull request Jun 10, 2019

fix(jest-haste-map): fix missing import statement #8548

Closed

lucasfcosta mentioned this pull request Jul 2, 2019

Fix mock reassigning to previously unexisting method #8631

Merged

github-actions bot added the Stale label Sep 8, 2022

github-actions bot closed this Oct 8, 2022

github-actions bot locked as resolved and limited conversation to collaborators Nov 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use testPathPattern as a filter on top of findRelatedTests #8311

Use testPathPattern as a filter on top of findRelatedTests #8311

mackness commented Apr 11, 2019 •

edited

Loading

jeysal commented Apr 11, 2019

jeysal Apr 11, 2019

mackness Apr 11, 2019

codecov-io commented Apr 12, 2019

jeysal commented Apr 13, 2019

mackness commented Apr 13, 2019

mackness commented Apr 17, 2019

jeysal commented Apr 18, 2019

mackness commented Apr 18, 2019

mackness commented Apr 24, 2019

jeysal commented Jun 3, 2019

mackness commented Jun 3, 2019

jeysal commented Jun 4, 2019

SimenB commented Jun 4, 2019

github-actions bot commented Sep 8, 2022

github-actions bot commented Oct 8, 2022

github-actions bot commented Nov 8, 2022

Use testPathPattern as a filter on top of findRelatedTests #8311

Use testPathPattern as a filter on top of findRelatedTests #8311

Conversation

mackness commented Apr 11, 2019 • edited Loading

Summary

Test plan

jeysal commented Apr 11, 2019

jeysal Apr 11, 2019

Choose a reason for hiding this comment

mackness Apr 11, 2019

Choose a reason for hiding this comment

codecov-io commented Apr 12, 2019

Codecov Report

jeysal commented Apr 13, 2019

mackness commented Apr 13, 2019

mackness commented Apr 17, 2019

jeysal commented Apr 18, 2019

mackness commented Apr 18, 2019

mackness commented Apr 24, 2019

jeysal commented Jun 3, 2019

mackness commented Jun 3, 2019

jeysal commented Jun 4, 2019

SimenB commented Jun 4, 2019

github-actions bot commented Sep 8, 2022

github-actions bot commented Oct 8, 2022

github-actions bot commented Nov 8, 2022

mackness commented Apr 11, 2019 •

edited

Loading