refactor: strip <rootDir> from collectCoverageFrom values #5524

StephanBijzitter · 2018-02-11T18:59:52Z

Summary

All values given to collectCoverageFrom are relative to the root directory, so usage of <rootDir> results in invalid file paths. With these changes, <rootDir> will be silently replaced.

I ran into this issue because I did not read the docs and expected <rootDir> to be usable within all paths. When I realized using <rootDir> in collectCoverageFrom did not work, I raised an issue (#5499).

A contributor (@SimenB) suggested silently replacing the <rootDir> values, allowing it to be used. I took a look at the code that handles the configuration, found the place to patch it in and here we have the pull request!

Test plan

I added a unit test that asserts the old and new behaviour. They work properly in tandem. :-)

SimenB · 2018-02-11T19:04:23Z

This is failing flow, mind fixing it? Run yarn flow locally to see the error.

Can you update the changelog as well?

Ideally we would require this to use rootDir for consistency with other options. That'd be a breaking change though, so this approach should be fine.

StephanBijzitter · 2018-02-11T19:13:20Z

Yeah I just noticed the error from one of the CI instances, strangely enough it says the array may be undefined, but running the code through my head I see no possible way to have it be undefined. Either way, happy to change the code around a bit to see if that'll work.

As for the breaking change if we require rootDir, I guess this would be a nice stepping stone allowing people to start using rootDir before it's required!

As for the changelog: Yeah, I can update it! I didn't do so initially because I thought this would just be too small for the changelog. I'm inclined to include this under ### Chore & Maintenance, does that sound right to you? I wouldn't really call this a new feature just yet.

SimenB · 2018-02-11T19:23:04Z

packages/jest-config/src/normalize.js

-  } else {
-    value = options[key];
+      const parsed = JSON.parse(options[key]);
+      Array.isArray(parsed) && (value = parsed);


ugh. Can we just use an if here?

if (Array.isArray(parsed)) { value = parsed; }

this is also the wrong logic, isn't it? I think the JSON.parse should be by its own in the try

Yes, we can! /obamamode off

I think the best would be to keep the logic you've added to line 163, and revert the other changes (feel free to exchange the || for an if, though :P)

Nah, the logic is solid. It's just that before my changing it was inside a try block assigning to the value and using the catch block to revert the value if it went wrong. This actually allowed an object to be passed though. For example:

normalizeCollectCoverageFrom({ collectCoverageFrom: {foo: 'bar'} })

Now, this is wrong of course, as it's supposed to take an array. As such, it would have entered the original if-statement that checked it to be an array. And since it's not an array, it would be parsed by JSON.parse, which would in turn throw an error as it's being given an object. The result would be the value still not being an array.

As such, the logic after the refactoring will always assure the result is an array. So while I could return the original code, I would rather not.

With the original code, this would be returned:

{ collectCoverageFrom: {foo: 'bar'} }

With the new code, this would be returned instead:

{ collectCoverageFrom: [] }

Reading through the code again, if the JSON string is not a representation of an array, this would still have an object as the value. I'll return the original 😆

All values given to collectCoverageFrom are relative to the root directory, so usage of <rootDir> results in invalid file paths. With these changes, <rootDir> will be silently replaced. ISSUES CLOSED: #5499

SimenB

Thanks!

codecov-io · 2018-02-11T19:58:27Z

Codecov Report

Merging #5524 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5524   +/-   ##
=======================================
  Coverage   61.68%   61.68%           
=======================================
  Files         213      213           
  Lines        7144     7144           
  Branches        4        3    -1     
=======================================
  Hits         4407     4407           
  Misses       2736     2736           
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-config/src/normalize.js	`93.25% <ø> (ø)`	⬆️

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 e9bf143...be35482. Read the comment docs.

thymikee · 2018-02-11T20:35:59Z

.gitignore

@@ -4,6 +4,8 @@
 *~
 /examples/*/node_modules/

+/.idea


@SimenB we care to remove this or just leave it alone?

I've got it in my global gitignore. doesn't hurt, though?

Doesn't hurt. People have opinions on the topic (local vs global gitignore), but it's not a big deal to me too so meh 🤷‍♂️ .

thymikee

Since we don't expect it to be relative pattern now, we can adjust the description in docs and CLI flag :)

github-actions · 2021-05-12T21:06:39Z

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.

facebook-github-bot added the CLA Signed ✔️ label Feb 11, 2018

SimenB reviewed Feb 11, 2018

View reviewed changes

StephanBijzitter added 3 commits February 11, 2018 20:41

refactor: strip <rootDir> from collectCoverageFrom values

09d44e0

All values given to collectCoverageFrom are relative to the root directory, so usage of <rootDir> results in invalid file paths. With these changes, <rootDir> will be silently replaced. ISSUES CLOSED: #5499

chore: ignore JetBrains configuration files

875d9cf

refactor: wrap mapping in a nullcheck

be35482

SimenB approved these changes Feb 11, 2018

View reviewed changes

thymikee reviewed Feb 11, 2018

View reviewed changes

thymikee approved these changes Feb 11, 2018

View reviewed changes

Update .gitignore

c7acd4c

cpojer merged commit 2d2fb5b into jestjs:master Feb 11, 2018

renovate bot mentioned this pull request Feb 14, 2018

fix(deps): update dependency jest to ^22.0.0 manekinekko/devoxx-assistant#3

Open

thymikee mentioned this pull request Feb 17, 2018

Jest 16: coverage empty with collectCoverageFrom #1849

Closed

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: strip <rootDir> from collectCoverageFrom values #5524

refactor: strip <rootDir> from collectCoverageFrom values #5524

StephanBijzitter commented Feb 11, 2018 •

edited

Loading

SimenB commented Feb 11, 2018

StephanBijzitter commented Feb 11, 2018 •

edited

Loading

SimenB Feb 11, 2018

SimenB Feb 11, 2018

StephanBijzitter Feb 11, 2018

SimenB Feb 11, 2018

StephanBijzitter Feb 11, 2018 •

edited

Loading

StephanBijzitter Feb 11, 2018

SimenB left a comment

codecov-io commented Feb 11, 2018

thymikee Feb 11, 2018

SimenB Feb 11, 2018

thymikee Feb 11, 2018

thymikee left a comment

github-actions bot commented May 12, 2021

refactor: strip <rootDir> from collectCoverageFrom values #5524

refactor: strip <rootDir> from collectCoverageFrom values #5524

Conversation

StephanBijzitter commented Feb 11, 2018 • edited Loading

Summary

Test plan

SimenB commented Feb 11, 2018

StephanBijzitter commented Feb 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephanBijzitter Feb 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB left a comment

Choose a reason for hiding this comment

codecov-io commented Feb 11, 2018

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thymikee left a comment

Choose a reason for hiding this comment

github-actions bot commented May 12, 2021

StephanBijzitter commented Feb 11, 2018 •

edited

Loading

StephanBijzitter commented Feb 11, 2018 •

edited

Loading

StephanBijzitter Feb 11, 2018 •

edited

Loading