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

Exclude setup files from coverage #7790

Merged
merged 10 commits into from
Feb 4, 2019

Conversation

lekterable
Copy link
Contributor

Summary

Fixes #7761

shouldInstrument function will return false if the file is one of the setupFiles or setupFilesAfterEnv.

Test plan

Added unit tests

@SimenB
Copy link
Member

SimenB commented Feb 3, 2019

This is great, thanks! Can you also add globalSetup and globalTeardown?

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?

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.

Looking good!

}

if (
config.setupFilesAfterEnv &&
Copy link
Collaborator

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

Copy link
Contributor Author

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?

Copy link
Collaborator

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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?

Copy link
Contributor Author

@lekterable lekterable Feb 3, 2019

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?

Copy link
Collaborator

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)

Copy link
Contributor Author

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!

Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SimenB comment added

@thymikee
Copy link
Collaborator

thymikee commented Feb 3, 2019

@SimenB not sure about that either, but I lean towards ignoring Jest-related files anyway, like with coverageIgnorePatterns. What's the point of having coverage for setup file? I can imagine someone doing that, but have never encounter anything like that.

@lekterable
Copy link
Contributor Author

@SimenB checks for globalSetup and globalTeardown added

@lekterable
Copy link
Contributor Author

Should I update the snapshots or leave it as it is?

@thymikee
Copy link
Collaborator

thymikee commented Feb 3, 2019

Yes, please :)

@jeysal
Copy link
Contributor

jeysal commented Feb 3, 2019

I'm fine with still ignoring setup files even if they are included in the coverage pattern - the coverageReport e2e test will need updating though (not just snapshot updates), otherwise "collects coverage only from specified file" will collect from 0 files and "collects coverage only from multiple specified files" will collect from 1 file. Should use a different collectCoverageFrom pattern than setup.js then.

CHANGELOG.md Outdated Show resolved Hide resolved
@lekterable
Copy link
Contributor Author

@jeysal tests updated

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.

This is great, thank you!

@SimenB SimenB merged commit 012820e into jestjs:master Feb 4, 2019
@lekterable lekterable deleted the setup-files-coverage branch February 4, 2019 09:28
captain-yossarian pushed a commit to captain-yossarian/jest that referenced this pull request Jul 18, 2019
@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.

setup{Files,FilesAfterEnv} included in coverage report
5 participants