Skip to content

Fix 'flow-coverage-report' command. #13393

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

Closed
wants to merge 1 commit into from

Conversation

elas7
Copy link
Contributor

@elas7 elas7 commented Aug 14, 2018

flow-coverage-report stopped working after Flow was set to run for each renderer separately (#12846). It happened because flow-coverage-report calls flow coverage, and that command doesn't work without a .flowconfig in a parent directory of every file checked.

This is fixed by creating a temporary .flowconfig in the project root before running flow-coverage-report.

This fix also allows to move .flowcoverage out of the project root, and to select a renderer before generating the coverage report.

Internally, this is done by rewriting writeConfig() to accept a rootFolder argument. If rootFolder is true, a config is made in the root folder, with the right relative paths.

Before
before

After
after

`flow-coverage-report` stopped working after Flow was set to run for each renderer separately (facebook#12846). It happened because `flow-coverage-report` calls `flow coverage`, and that command doesn't work without a `.flowconfig` in a parent directory of every file checked.

This is fixed by creating a temporary `.flowconfig` in the project root before running `flow-coverage-report`.

This fix also allows to move `.flowcoverage` out of the project root, and to select a renderer before generating the coverage report.

Internally, this is done by rewriting `writeConfig()` to accept a `rootFolder` argument. If `rootFolder` is true, a config is made in the root folder, with the right relative paths.
@gaearon
Copy link
Collaborator

gaearon commented Aug 14, 2018

Thanks for the PR! The libs substitution is more complex than I would like.. Ideally we want to avoid as much abstraction as we can there.

Maybe let's just remove this script? We don't really use it. Or find some other solution that doesn't involve making the config more abstract.

@elas7
Copy link
Contributor Author

elas7 commented Aug 14, 2018

Yes, makes sense.

I don't think there's a way to solve this without adding complexity to .flowconfig's generation. I'll make a PR to remove the script.

elas7 added a commit to elas7/react that referenced this pull request Aug 14, 2018
`flow-coverage-report` stopped working after Flow was set to run for each renderer separately (facebook#12846). As discussed in facebook#13393, this is hard to fix without adding complexity to `.flowconfig`'s generation.
@elas7 elas7 closed this Aug 14, 2018
gaearon pushed a commit that referenced this pull request Aug 14, 2018
`flow-coverage-report` stopped working after Flow was set to run for each renderer separately (#12846). As discussed in #13393, this is hard to fix without adding complexity to `.flowconfig`'s generation.
@elas7 elas7 deleted the fix-flow-coverage branch August 14, 2018 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants