Skip to content

Implement code coverage heat map#145

Merged
nikolay-borzov merged 9 commits into
danvk:masterfrom
joshribakoff:master
Dec 30, 2019
Merged

Implement code coverage heat map#145
nikolay-borzov merged 9 commits into
danvk:masterfrom
joshribakoff:master

Conversation

@joshribakoff

@joshribakoff joshribakoff commented Nov 18, 2019

Copy link
Copy Markdown
Contributor

In Google Chrome, you can collect code coverage stats while loading/browsing around your local build (documented here https://developers.google.com/web/tools/chrome-devtools/coverage).

Screen Shot 2019-11-07 at 10 42 36 AM

Screen Shot 2019-11-07 at 10 42 47 AM

This PR adds an [optional] --coverage flag to the source-map-explorer, which takes in a JSON export of the Chrome code coverage.

Passing the coverage data will attempt to color code the heat map like so (I obscured the file names for this screenshot):

Screen Shot 2019-11-18 at 1 16 25 PM

This allows you to find the code that is not strictly needed for the initial page load & helps to identify the ideal ways to code split.

Coverage decreased (-6.4%) to 91.247% when pulling eb195c6 on joshribakoff:master into 98b13b2 on danvk:master.

FYI, my actual business logic should have 100% coverage, but I suspect this is due to adding lines of code to cli.ts, explore.ts, api.ts that basically just plumbs the path of the json down to my function (which is 100% tested).

Also, FYI I've taken care to ensure the algorithm scales in O(n) where n is the size of the bundle, by using a ratcheting pointer technique, rather than using a naive nested loop O(n^2) solution.

Also, your tests do not pass on my machine & fail with the below errors, I commented these 2 tests out locally:


  63 passing (2s)
  2 failing

  1) api
       explore
         when output filename specified
           should save html to file:
     Error: Unable to save HTML to file: EEXIST: file already exists, mkdir '.'
      at saveOutputToFile (src/output.ts:67:13)
      at explore (src/api.ts:78:3)
      at <anonymous>

  2) api
       explore
         when output filename specified
           should save html to file creating nested directories:
     Error: Unable to save HTML to file: ENOENT: no such file or directory, mkdir './tmp/nested'
      at saveOutputToFile (src/output.ts:67:13)
      at explore (src/api.ts:78:3)
      at <anonymous>


@coveralls

coveralls commented Nov 18, 2019

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-6.1%) to 92.701% when pulling 9dd5dc3 on joshribakoff:master into 4b95f6e on danvk:master.

@danvk

danvk commented Nov 18, 2019

Copy link
Copy Markdown
Owner

I'll defer to @nikolay-borzov for the code review, but I wanted to say that this is a great idea!

Out of curiosity, given the screenshot you included, how would you split/pare down your bundle? It doesn't look like there are very many red modules at the top level.

@joshribakoff

Copy link
Copy Markdown
Contributor Author

@danvk thanks!

Those red boxes correspond to code that would only be executed if the user took some action, or if some condition was met. For example it may be a component inside of a dropdown the user never interacted with, or components that are only needed if the user opens a modal. In cases where the parent is green but the boxes inside are red, that means maybe some "initialization" logic ran, but the inner code never ran. Maybe we mounted a button, but not the other components in that module that are only needed if & when the user clicks the button, in that case I would have the button trigger the rest of the code to load.

Another example of this is code that attaches a function to window that never actually gets called, or modules that just re-export react components, the barrel export will always run but sometimes the react component never actually gets mounted. A popular tool Sentry that's used for error logging has a big SDK and it doesn't need to run unless there is actually an error to report, so they have a "lazy loader" that registers a listener, buffers errors, and loads the code to process & submit the errors on demand (only if and when an error happens). Things like this can split out, even if not at the top level.

For those features we deem as less important, we would evaluate if they need to be in the critical path, and if not we would split them out & defer them until some condition is met (eg. the user has interacted, or the rest of the page finished loading, or some event has actually fired).

I actually have another tool I'm working on, for a followup PR, which would color code according to how frequently the code changes according to git.

I would recommend (if code splitting manually) to try to create 3 chunks for a heavy page, more if using http2:

  • The critical / minimal JS that is needed for initial page load, try to have 20% or less "dead code" served on initial page load. Try to have all of this referenced in your .html so it starts downloading right away, as opposed to dynamically loaded "on demand" with an import(). Make use of webpack entry points so webpack can omit code not needed on a given page. You don't want 100% coverage, because that would mean you split too aggressively which entails tradeoffs. There's always going to be some dead code (for example error handlers, but most of the time there is no error)

  • JS that changes more often should be in a separate chunk so it doesn't invalidate the lesser changing code. This code may be loaded in a <script /> tag in the initial page load, or on demand.

  • The features that are considered "secondary" because not many users use them, or they can be prioritized lower so they load only after the more important features.

The heatmap feature in this PR helps you identify the code that is needed for a fast initial page load (green), as well as helps to identify the code that can be [potentially] deferred because it doesn't run until the user interacts with some feature (red).

Comment thread src/api.ts Outdated
Comment thread src/api.ts Outdated
Comment thread src/explore.ts Outdated
Comment thread src/explore.ts
Comment thread src/find-ranges.ts Outdated
Comment thread src/html.ts Outdated
Comment thread src/html.ts Outdated
Comment thread tests/find-ranges.test.ts Outdated
Comment thread tests/__snapshots__/api.test.ts.snap Outdated
@joshribakoff

Copy link
Copy Markdown
Contributor Author

@nikolay-borzov feedback has been incorporated or addressed, and conflicts fixed. Would you mind either meeting me halfway on these other 2 items, or giving some reasoning why you desire those changes? Thanks for the review!

@nikolay-borzov nikolay-borzov self-assigned this Nov 26, 2019
@nikolay-borzov nikolay-borzov added this to the 2.2.0 milestone Nov 26, 2019
Comment thread tests/find-ranges.test.ts Outdated
@joshribakoff

Copy link
Copy Markdown
Contributor Author

@nikolay-borzov I've gone ahead & fixed those final items per your request, thanks again for the review!

@nikolay-borzov

Copy link
Copy Markdown
Collaborator

Thanks for your contribution! I'll incorporate this feature when I'm through 2.1.2 issues

- [ESLint] Require empty line after `const`/`let`
- [ESLint] Enable `object-shorthand`
- Move code coverage related methods to `coverage.ts`
- Add `CannotOpenCoverageFile` and `NoCoverageMatches` errors
- Replace `FileSizeMap` with `FileData` so that additional data can be stored along with mapping size
- Implement mapping coverages to bundles by comparing coverage URL with bundle name.
- Convert coverage ranges (one-line) to per line range to ease matching with mapping ranges
- When calculating file sizes first get mapping ranges with sources, then merge consecutive ranges and then calculate bytes
- Calculate cover bytes size
- Fix bytes calculation - get bytes length from source ranges' string rather than columns count. One column can have byte length 4 bytes (e.g. 🍰)
- Move mapping reference EOL check before invalid mapping column check
- Add `rewiremock` to mock dependencies during tests
- Add `mappedBytes` value to result
@nikolay-borzov nikolay-borzov changed the title Implements code coverage heat map Implement code coverage heat map Dec 30, 2019
@nikolay-borzov nikolay-borzov merged commit 9c3195d into danvk:master Dec 30, 2019
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.

4 participants