Exclude setup files from coverage

[lekterable]

Summary

Fixes #7761

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

Test plan

Added unit tests

[SimenB]

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?

[thymikee]

Looking good!

[thymikee]

setupFilesAfterEnv and setupFiles are normalized to return empty array, so this extra check is not necessary

[lekterable]

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

[thymikee]

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.

[lekterable]

yeah sure, I'll try to take care of this

[lekterable]

@thymikee I've added the pragma and types, how would I make it use the normalized config tho?

[lekterable]

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?

[thymikee]

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)

[lekterable]

agreed, I will try to take care of this in a new PR, thanks for the help!

[SimenB]

Could you add TODO comments so we don't forget?

[lekterable]

@SimenB comment added

[thymikee]

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

@SimenB checks for globalSetup and globalTeardown added

[lekterable]

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

[thymikee]

Yes, please :)

[jeysal]

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.

[lekterable]

@jeysal tests updated

[SimenB]

This is great, thank you!

[github-actions]

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.