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

--generateCoverageForFiles #4246

Closed

Conversation

aaronabramov
Copy link
Contributor

fixes #4245

Tests: 1 passed, 1 total
Snapshots: 0 total
Time: <<REPLACED>>
Ran all test suites matching /a.js/i.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is incorrect (also broken in master for --findRelatedTests)
i filed the issue #4247

// should only run a.js
expect(rest).toMatchSnapshot();
// coverage should be collected only for a.js
expect(stdout).toMatchSnapshot();
Copy link
Collaborator

Choose a reason for hiding this comment

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

btw, not really needed here, but recently I came up with more descriptive snapshot testing such scenarios:

describe(('--generateCoverageForFiles') => {
  test('summary', () => expect(summary).toMatchSnapshot())
  test('rest', () => expect(rest).toMatchSnapshot())
  test('stdout', () => expect(stdout).toMatchSnapshot())
})

This gives more descriptive snapshot names, together with error messages when diffing later (it's easy to loose context in large snapshots, and names are easier to understand than checking which number is for which snap)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wait.. are you running the subprocess outsude of test?
i agree that snapshots are hard to track!

// `--findRelatedTests '/rootDir/file1.js' --coverage --collectCoverageFrom 'file1.js'`
// where arguments to `--collectCoverageFrom` should be globs (or relative
// paths to the rootDir)
if (argv.generateCoverageForFiles) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we good that it's only for CLI?

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! i don't think we'd want to put it in a config, since it's a very interactive option :)

@aaronabramov
Copy link
Contributor Author

aaronabramov commented Aug 11, 2017

website build travis and appveyor seems to be failing for unrelated reasons :(


test('--generateCoverageForFiles', () => {
writeFiles(DIR, {
'.watchmanconfig': '',
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this really needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think if you have watchman installed on your computer (which we have at fb) it'll complain in stdout if this file is not present, which will in turn make it harder to snapshot the output :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe it's worth creating a wraper function createJestProject() that'll add it by default.
but at the same time i'd rather keep it explicit

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm for explicitness here, it's easier not to loose the context

@cpojer
Copy link
Member

cpojer commented Aug 11, 2017

I'm not opposed to adding this option, however I don't think the naming makes sense. It implies that it will collect coverage but does not imply that tests will be run.

How about --findRelatedTests file1 file2 --collectCoverageOnlyForPatterns

The latter argument will turn on collection for all patterns that are passed in to Jest. It'll collect coverage for file1 and file2, but not file3, for example. What do you think?

Also, I'm not sure how this speeds up arc diff at FB – why does the current solution with passing all the files twice not work?

@aaronabramov
Copy link
Contributor Author

  1. --collectCoverageOnlyForPatterns works! my main motivation for this though was the fact that i get asked almost every week the same question: "how do i generate coverage for a.js?" and every time i wish there was a command that says exactly that :) if i start explaining that to generate coverage for a.js you need to run all related tests and running only __tests__/a.test.js is not sufficient, which is very hard to grasp for people who are not very familiar with testing or coverage concepts.

  2. passing files twice works, however it reduces the number of files we can pass (string overflow that we've been running into recently). So that would work right now, but i wouldn't want it for the long term solution

@aaronabramov
Copy link
Contributor Author

maybe something like
--findRelatedTestsAndCollectCoverageFrom

@cpojer
Copy link
Member

cpojer commented Aug 14, 2017

  • Not generally a fan and. I guess I'm fine calling it --collectCoverageFor <files> which merges findRelatedTests and collectCoverageOnlyFrom. I don't actually like findRelatedTests either, because it doesn't imply that any tests are run.
  • If you are running into a string overflow now, you'll run into it again in the future, so we'll need another solution for this (stdin etc.).

All of this kind of hints at the cli becoming unwieldy and overly complex :( Do you have any ideas what we could do about that?

@michael-ciniawsky
Copy link

jest --coverage                   # All 
jest --coverage file.test.js ...  # Only for files/patterns given as arguments

?

@aaronabramov
Copy link
Contributor Author

@michael-ciniawsky we can't do this, because by default jest expect test patterns to be passed in, and we don't collect coverage for test files.
I'll close it for now because i don't have time to look into it, but i'll come back to it later!

@michael-ciniawsky
Copy link

michael-ciniawsky commented Aug 23, 2017

Ohh sry, I see what you mean 😛, should be e.g jest --coverage srcDir || srcFile.js (--collectCoverageFrom)? The general idea is, instead of introducing a --flag pass arguments whenever possible to avoid "polluting" the CLI Interface.

cpojer pushed a commit that referenced this pull request Mar 21, 2018
…Changed` and `--watch` scenarios (#5601)

* Jump start using the work from #4246

* checking how it looks without an extra flag

* makes more sense to colocate the test here

* remove mystery files

* remove references to the flag

* update docs

* add test case for --onlyChanged and --coverage

* update snapshot to what it should be

* apply collectCoverageFrom patterns after finding tests

* Collect possible coverage patterns while searching for tests

* improve test results

* hi there flow

* skip on windows

* will the windows build pass this time?

* fix test on windows

* remove underscore as it's no longer a private api

* polymorphing

* simplify test

* increase test coverage in jest-config/normalize

* make the linter happy again

* should use argv as it's not a config option

* test coverage for run_jest

* update snapshots

* remove object spread, replaced with Object.assign

* add changelog entry
@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 13, 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.

feature request: --findRelatedTests --coverage collects coverage for given files only
5 participants