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

🏗✨ Enforce a minimum of 80% code coverage for AMP source code #28013

Merged
merged 1 commit into from
Apr 24, 2020
Merged

🏗✨ Enforce a minimum of 80% code coverage for AMP source code #28013

merged 1 commit into from
Apr 24, 2020

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Apr 24, 2020

Background:

One of the AMP team's long-term goals is to improve release quality via automated testing. A few of the bugs we've found recently could have easily been caught by tests that were either missing or had been skipped.

Today, the AMP runtime code (not counting ads, infrastructure, third party, and test code) has a little more than 80% code coverage from unit tests as reported on this dashboard (click through for subdirectory-level numbers).

image

It's important to note that 100% code coverage does not guarantee that 100% of all code paths are tested. However, it still is worth detecting and addressing instances of code with no (or very low) coverage. For example, we have 20% coverage for builtins/amp-layout.js, <40% coverage for src/polyfills, and ~55% coverage for src/inabox. In addition, there are numerous runtime files with <50% coverage and dozens of extension files with <60% coverage.

Previous work:

In #27843, we enabled code coverage statuses for every AMP PR in informational mode.

For PRs that touch runtime code and have test coverage, we see a status like this:

image

For PRs that don't touch runtime code, we see a status like this:

image

PR highlights:

This PR implements the recommendation around code coverage from a recent design review (#27599 (comment)). We are moving from informational mode to blocking mode. Any PR that touches AMP runtime code and has lower than 80% patch-level coverage (i.e., coverage for the lines being changed) will have receive a failing check.

image

The 80% target was chosen in order to prevent PRs from lowering AMP's overall coverage. There is a further 1% threshold to prevent rounding errors, and to allow for some wiggle room.

Note that the status is not a required Github status (for now), so a PR that doesn't meet the coverage bar can still be merged (although this is not advised).

Local testing:

In order to tell how an in-flight PR is likely to affect patch-level code coverage, run the following command.

gulp unit --local_changes --coverage

After tests complete, you will see a full coverage report for the tests you just ran in a new browser tab.

Future work:

With this, we should see a gradual uptick in total code coverage as tests are added for code that is currently untested. As total coverage increases over time, we can increase the target values.

In addition, if we are able to figure out how to measure code coverage (#22682) for integration, end-to-end, and visual-diff tests, we can further add to the coverage measurements and further increase the thresholds.

@rsimha
Copy link
Contributor Author

rsimha commented Apr 24, 2020

/to @cramforce / @mrjoro for approval
/cc @ampproject/wg-approvers for information

@rsimha rsimha merged commit fe8e475 into ampproject:master Apr 24, 2020
@rsimha rsimha deleted the 2020-04-24-CodeCoverageThreshold branch April 24, 2020 18:06
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.

None yet

4 participants