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: Jest multi project runner still cannot handle exactly one project #8894

Merged
merged 1 commit into from
Nov 22, 2019

Conversation

kenrick95
Copy link
Contributor

@kenrick95 kenrick95 commented Aug 30, 2019

Summary

This PR adds a failing test case for #8860 but I haven't had any clue on how to fix it and I found a way to solve it, but I actually wonder if this is the best way to fix this. I guess I need input from the maintainers.

There are failing tests when I do the following change

-  if (
-    projects.length > 1 ||
-    (projects.length && typeof projects[0] === 'object')
-  ) {
+  if (projects.length > 0) {

The main issue was that at readConfigs, when calling readConfig, skipArgvConfigOption should only be true if the project is not "global". By global, I mean, it is not added from getProjectListFromCLIArgs using process.cwd. That's why I changed getProjectListFromCLIArgs to also return isGlobalProject.

Update: the implementation have changed based on suggestion during code review: #8894 (comment)

Test plan

Added unit test case :)

@kenrick95 kenrick95 changed the title WIP: Attempt to fix #8860 Attempt to fix #8860 Sep 5, 2019
@kenrick95 kenrick95 marked this pull request as ready for review September 5, 2019 10:20
@kenrick95 kenrick95 changed the title Attempt to fix #8860 fix: Jest mutli-project-runner still cannot handle exactly one project Sep 5, 2019
@codecov-io
Copy link

codecov-io commented Sep 6, 2019

Codecov Report

Merging #8894 into master will increase coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #8894      +/-   ##
=========================================
+ Coverage   65.09%   65.1%   +0.01%     
=========================================
  Files         278     278              
  Lines       11860   11860              
  Branches     2922    2922              
=========================================
+ Hits         7720    7722       +2     
  Misses       3511    3511              
+ Partials      629     627       -2
Impacted Files Coverage Δ
packages/jest-config/src/index.ts 11.94% <0%> (ø) ⬆️
packages/expect/src/utils.ts 96.2% <0%> (+1.26%) ⬆️

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 edde511...1b52598. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Nov 9, 2019

Sorry about the late feedback... Would you be able to rebase, just so we are sure it still passes CI?

@kenrick95
Copy link
Contributor Author

Sure no problem 😁

packages/jest-config/src/index.ts Outdated Show resolved Hide resolved
@kenrick95 kenrick95 changed the title fix: Jest mutli-project-runner still cannot handle exactly one project fix: Jest multi project runner still cannot handle exactly one project Nov 22, 2019
Add failing test case

Explicitly distinguish if project has been added using process.cwd

Fix CI errors

Add back old test

Apply solution without isGlobalProject

Apply suggestion from code review

Add changelog
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Thanks!

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

5 participants