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

refactor: strip <rootDir> from collectCoverageFrom values #5524

Merged
merged 4 commits into from
Feb 11, 2018
Merged

refactor: strip <rootDir> from collectCoverageFrom values #5524

merged 4 commits into from
Feb 11, 2018

Conversation

StephanBijzitter
Copy link
Contributor

@StephanBijzitter StephanBijzitter commented Feb 11, 2018

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
Copy link
Member

SimenB commented Feb 11, 2018

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
Copy link
Contributor Author

StephanBijzitter commented Feb 11, 2018

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.

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

Choose a reason for hiding this comment

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

ugh. Can we just use an if here?

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we can! /obamamode off

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

@StephanBijzitter StephanBijzitter Feb 11, 2018

Choose a reason for hiding this comment

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

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: []
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
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!

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

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

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

/.idea
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 🤷‍♂️ .

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

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

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

6 participants