Skip to content

Conversation

@nmote
Copy link
Contributor

@nmote nmote commented Jun 6, 2016

Running flow coverage on the file below, in the Nuclide codebase, occupies the Flow server for two minutes on my machine:

Nuclide/pkg/nuclide-reprint-js/lib/printers/complex/printLiteral.js

This is because coverage is implemented using the dump-types server command, which forces all of the types to be stringified and sent across the process boundary. The file above, for whatever reason, has types that are ridiculously long when stringified. To solve this, I've added a new coverage command to the server, which simply returns a boolean depending on whether the region is covered or not. With this change, running flow coverage on the file above is basically instantaneous, and the output is identical to the output without this change.

I will follow up with another PR to add detailed coverage information to the JSON output, so Nuclide can switch from dump-types to coverage for its coverage display. Then we can get away from this pathological behavior.

Of course, this PR doesn't fix dump-types. But it solves my immediate problem and seems like the right long-term call for coverage anyway.

@ghost ghost added the CLA Signed label Jun 6, 2016
@mroch
Copy link
Contributor

mroch commented Jun 6, 2016

@facebook-github-bot shipit

@ghost
Copy link

ghost commented Jun 7, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review.

@ghost ghost closed this in f0d8f49 Jun 7, 2016
@nmote
Copy link
Contributor Author

nmote commented Jun 7, 2016

FYI, I just ran coverage over all the files in the Nuclide pkg directory and summed up the results, and they were the same both before and after this command (although the before took about 2 hours to run, and after took about 2 minutes):

covered:   202364
uncovered: 19411
percentage: 91.25%

This pull request was closed.
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