Skip to content

Instrument Repo for Code Coverage #268

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

Merged
merged 13 commits into from
Nov 16, 2017
Merged

Instrument Repo for Code Coverage #268

merged 13 commits into from
Nov 16, 2017

Conversation

jshcrowthe
Copy link
Contributor

@jshcrowthe jshcrowthe commented Oct 26, 2017

This is going to probably take a little extra time (we have to get the extra CI integration approved) but having this instrumented will give us a path to improving our coverage.

Instrumenting files

Browser

All of our browser builds running through karma-webpack are instrumented via istanbul-instrumenter-loader. We then dump this info for each component into the component coverage/browser dir by browser.

Node

All of our node builds are instrumented via nyc which is a tool provided by the istanbul folks. Unfortunately, karma is not supported in the same way so we needed two tools. Coverage for this tool is dumped in the coverage/node directory.

Merging lcov.info Files

After collecting lcov.info files (i.e. the coverage reports) for each file we leverage lcov-result-merger to consolidate to a single file that we can then pipe to CI.

I am going to leave the CI integration (i.e. coveralls) in as @vikrum is currently evaluating if we can get access here.

@schmidt-sebastian
Copy link
Contributor

Approved for RTDB. Got the coverage numbers that I needed. Thanks!

Copy link
Contributor

@wilhuff wilhuff left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -27,9 +27,18 @@ module.exports = {
use: 'ts-loader'
},
{
test: /\.js$/,
use: ['source-map-loader'],
test: /\.[tj]sx?$/,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just a standard configuration now or are we really doing jsx and tsx stuff? In contrast in other locations we're still using just straight .ts file extensions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just a standard thing, we have no need for jsx/tsx so it isn't that big of a deal I don't think.

@jshcrowthe jshcrowthe requested a review from sphippen as a code owner November 16, 2017 21:24
@jshcrowthe jshcrowthe merged commit 7db74e6 into master Nov 16, 2017
@jshcrowthe jshcrowthe deleted the coverage branch November 16, 2017 23:26
@mikelehen
Copy link
Contributor

FYI- It seems like you upgraded prettier in this PR, which caused a bunch of extra formatting changes, which is causing me merge conflicts in a different branch. Going forward it would be helpful if:

  1. We don't upgrade prettier except in an isolated PR that only contains the upgrade (and the fallout formatting changes)
  2. We coordinate this across the team so that folks are aware (and potentially we get large PRs checked in before the upgrade to avoid conflicts).

@firebase firebase locked and limited conversation to collaborators Oct 25, 2019
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.

8 participants