-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Exclude setup files from coverage #7790
Conversation
a795564
to
8211398
Compare
This is great, thanks! Can you also add This also broke a couple of old tests. I'm not sure what the behavior should be if the files are explicitly asked coverage for. @thymikee thoughts? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
} | ||
|
||
if ( | ||
config.setupFilesAfterEnv && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setupFilesAfterEnv
and setupFiles
are normalized to return empty array, so this extra check is not necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thymikee I think they are not normalized for unit tests, just checked and got TypeError: Cannot read property 'some' of undefined
, do you want me to leave the check here or provide a default value somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's because shouldInstrument
test file is not typed. Wanna try to add @flow
pragma to it and make it use normalized config as it actually expects? If that's too tedious, just pass default values for these specific tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah sure, I'll try to take care of this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thymikee I've added the pragma and types, how would I make it use the normalized config tho?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is that currently, test cases overwrite the default config when they pass their own config object to testShouldInstrument
so it still throws a Cannot read property 'some' of undefined
error. How about making the testShouldInstrument
in should_instrument.test.js
merge the default config and the one from args instead of overwriting it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldInstrument
expects every config
to be normalized. As I said, it's probably to much of a hassle and not in scope of this PR, so I'm good with leaving this as is (although it seems like an area for some small speedup, so definitely worth following up later)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, I will try to take care of this in a new PR, thanks for the help!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add TODO
comments so we don't forget?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SimenB comment added
@SimenB not sure about that either, but I lean towards ignoring Jest-related files anyway, like with |
@SimenB checks for |
Should I update the snapshots or leave it as it is? |
Yes, please :) |
I'm fine with still ignoring setup files even if they are included in the coverage pattern - the |
3d93884
to
4d8ae5c
Compare
@jeysal tests updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, thank you!
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. |
Summary
Fixes #7761
shouldInstrument
function will return false if the file is one of thesetupFiles
orsetupFilesAfterEnv
.Test plan
Added unit tests