Skip to content

Add coverage to packages beyond contracts#1297

Closed
rajivpo wants to merge 4 commits intoethereum-optimism:developfrom
rajivpo:rajiv/add-coverage
Closed

Add coverage to packages beyond contracts#1297
rajivpo wants to merge 4 commits intoethereum-optimism:developfrom
rajivpo:rajiv/add-coverage

Conversation

@rajivpo
Copy link
Contributor

@rajivpo rajivpo commented Jul 28, 2021

Description
Added the ability to surface test coverage metrics for all relevant packages. This is the first of hopefully a few PRs I want to submit to improve test coverage across various packages (probably starting with core-utils as it's the easiest) and figured having the ability to surface where coverage is lacking in an automated fashion would be helpful for development.

I've also seen repos hook up coverage numbers to their homepages, and if this is a desired feature, I'm happy to investigate. There's also probably an opportunity to ensure that coverage isn't decreasing as a pre-commit hook, and am happy to spend time implementing that would be helpful.

Additional context

Metadata

@changeset-bot
Copy link

changeset-bot bot commented Jul 28, 2021

🦋 Changeset detected

Latest commit: acccf17

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@eth-optimism/batch-submitter Patch
@eth-optimism/core-utils Patch
@eth-optimism/data-transport-layer Patch
@eth-optimism/message-relayer Patch
@eth-optimism/smock Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added 2-reviewers A-op-batcher Area: op-batcher A-pkg-core-utils Area: packages/core-utils labels Jul 28, 2021
@tynes
Copy link
Contributor

tynes commented Aug 2, 2021

Hey @rajivpo thanks for the PR! Do you mind explaining a bit about how nyc is used? Perhaps a blurb in the top level README. It looks like this PR adds the ability to run nyc against each of the different packages - it would be nice to have its output show up in the PR or something like that. I believe we use codecov already for the contracts, it looks like there is an integration: https://github.com/istanbuljs/nyc/blob/master/docs/setup-codecov.md

@tynes
Copy link
Contributor

tynes commented Aug 2, 2021

Do you also mind squashing your commits into a single commit?

@github-actions github-actions bot added the A-ci label Aug 3, 2021
@codecov-commenter
Copy link

codecov-commenter commented Aug 3, 2021

Codecov Report

Merging #1297 (8dc7276) into develop (1f1fff9) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1297   +/-   ##
========================================
  Coverage    86.05%   86.05%           
========================================
  Files           49       49           
  Lines         1936     1936           
  Branches       307      307           
========================================
  Hits          1666     1666           
  Misses         270      270           
Impacted Files Coverage Δ
...c-ethereum/libraries/standards/L2StandardERC20.sol 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33cb902...8dc7276. Read the comment docs.

@rajivpo rajivpo force-pushed the rajiv/add-coverage branch from 24be309 to dce4c3f Compare August 3, 2021 03:28
env:
CC_SECRET: ${{ secrets.CC_SECRET }}

# A hack that allows running a job only if a specific directory changed.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured i could remove this entire block now that there's coverage across all ts-packages, but if there's a good reason to keep this, happy to add back in.

@rajivpo rajivpo force-pushed the rajiv/add-coverage branch from 9ab1df6 to dce4c3f Compare August 3, 2021 03:47
@rajivpo
Copy link
Contributor Author

rajivpo commented Aug 10, 2021

@tynes does this suffice?

@rajivpo
Copy link
Contributor Author

rajivpo commented Aug 20, 2021

Should have a bit more time this week - will continue adding to coverage to packages

@elenadimitrova
Copy link
Contributor

Thanks for the contribution! In line with what @tynes said above, can we also add the Codecov report run on the respective package readme, same way we do for contracts https://github.com/ethereum-optimism/optimism/tree/develop/packages/contracts
Also we need to add checks for the branch coverage to CI for each package and set it to not drop below what we currently have. I think improving coverage after that can be done in separate PRs to make this and subsequent work easier to review.

@elenadimitrova
Copy link
Contributor

@rajivpo heya, just looking at the last commit and seeing that codecov is now just on the main readme, as opposed to what we agreed above which was to add the Codecov report run on each of the respective package readme. Basically I personally think that coverage per package is more meaningful than overall monorepo coverage. Sorry if my comment above was what confused you!

@rajivpo
Copy link
Contributor Author

rajivpo commented Aug 30, 2021

oh okok yes this makes a lot more sense, I think this can be done with flags via codecov, but will need to double check

@rajivpo
Copy link
Contributor Author

rajivpo commented Sep 2, 2021

Given how messy this is, I'm going to close this out as a much cleaner version of this is in development in #1412

@rajivpo rajivpo closed this Sep 2, 2021
theochap pushed a commit that referenced this pull request Dec 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-op-batcher Area: op-batcher A-pkg-core-utils Area: packages/core-utils

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants