Skip to content
This repository has been archived by the owner on Apr 27, 2022. It is now read-only.

Add coverage #429

Merged
merged 7 commits into from
Feb 23, 2021
Merged

Add coverage #429

merged 7 commits into from
Feb 23, 2021

Conversation

fedgiac
Copy link
Contributor

@fedgiac fedgiac commented Feb 10, 2021

Adds coverage reporting to this project.

Note that coverage testing skips all end to end tests. The motivation is twofold:

  1. During coverage, hardhat-deploy "forgets" that we are deploying with a proxy and does not retrieve the right deployment information.
  2. Working around 1, deployment fails claiming that the code of the contract deployed in the constructor (allowance manager) is too large (ref. EIP-170). This check (from hardhat-deploy bypasses the Hardhat network flag allowUnlimitedContractSize.

Test Plan

Instructions in readme.

Expected result:

coverage

Copy link
Contributor

@fleupold fleupold left a comment

Choose a reason for hiding this comment

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

Nice coverage already!

Maybe it would be cool to run coverage on CI and upload the results to https://coveralls.io/ so that we get the nice little Badge on our repo readme (here is the line that did it in GPv1)

Coverage Status

Copy link
Contributor

@nlordell nlordell left a comment

Choose a reason for hiding this comment

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

Sweet sauce! Now we just need to upload the coverage to coveralls or something and setup the badge.

I'm also more than happy to merge this PR as-is even with the pragma abicode v2 issue in the solidity coverage plugin. We will just have to skip v0.7.14 and close the Dependabot PR.

hardhat.config.ts Outdated Show resolved Hide resolved
hardhat.config.ts Show resolved Hide resolved
fedgiac and others added 2 commits February 11, 2021 14:20
Co-authored-by: Nicholas Rodrigues Lordello <nlordell@gmail.com>
@fedgiac
Copy link
Contributor Author

fedgiac commented Feb 12, 2021

Coveralls is easy to implement as an extra check that fails in case coverage decreases. It would look like this:

coverage-check

Do we want this @nlordell, @fleupold?

@fleupold
Copy link
Contributor

I think it would be nice to have this check but maybe make it non-blocking? On the other hand as admins we can still merge requests that have a blocking check not pass.

@fedgiac
Copy link
Contributor Author

fedgiac commented Feb 12, 2021

Here is the result. Successful checks are collapsed by default, so it needs to be expanded.
nonblocking-coverage-check
This is done in the Coveralls website, repo -> settings -> "Coverage Decrease Threshold for failure": 100%.

A thing to note with using Coveralls is that the CI line creating the check requires the CI authentication token, so we would be giving unlimited access to this repository to Coveralls.

@fedgiac
Copy link
Contributor Author

fedgiac commented Feb 15, 2021

I consider this PR ready: the only changes that I still intend to include are these. I'm not implementing them without confirmation since doing so would give (a malicious) Coveralls unfettered access to this repo (the command receives a GitHub token that allows it to make almost arbitrary git changes). The alternative would be implementing this manually with Coveralls's API or without Coveralls. In GPv1 a malicious Coveralls would have access to secret CI env variables but was not able to change the repo itself. Wdyt @fleupold, @nlordell?

@nlordell
Copy link
Contributor

Do you know what permissions the Coveralls app needs? To me it looks like it just wants to authenticate that it is the repo, so maybe you can create a "personal access token" for Coveralls?

If you need a token that requires permissions that aren't available in the GITHUB_TOKEN, you can create a personal access token and set it as a secret in your repository:

https://docs.github.com/en/actions/reference/authentication-in-a-workflow#permissions-for-the-github_token

@fedgiac
Copy link
Contributor Author

fedgiac commented Feb 15, 2021

To my understanding, it needs some write access because it adds a new check on our repo. Also, you cannot create a custom personal access token for a single project, it can only inherit its permissions from an actual account. The only way down this route I see is creating a dedicated GitHub account, add it to the list of people allowed to make changes to the repo and then use it to create a personal access token.

@nlordell
Copy link
Contributor

nlordell commented Feb 15, 2021

Boo!

@nlordell
Copy link
Contributor

I guess its similar to how we give apps like Mergify repository access, so I'm OK with it.

@nlordell
Copy link
Contributor

This is done in the Coveralls website, repo -> settings -> "Coverage Decrease Threshold for failure": 100%.

I think the way the repo is set up only the Mergify rule and GitHub action prevent merging:

image

Additionally, the mergify configuration only checks the GitHub action check:

conditions:
- "#approved-reviews-by>=1"
- status-success~=^test \([0-9]+\.x, ubuntu-latest\)$
- base=main
- label=merge when green

I think its better for coverage decreases to show a big red X so we don't miss it, while, given the current configuration, should not prevent merging.

@fedgiac fedgiac marked this pull request as ready for review February 22, 2021 17:49
@fedgiac
Copy link
Contributor Author

fedgiac commented Feb 22, 2021

PR is ready without changes from the draft. Only exception is that I updated solidity-coverage to the latest version.
Thanks to @cgewecke for supporting this great coverage tool!

@fedgiac fedgiac added merge when green Merge when green! and removed merge when green Merge when green! labels Feb 22, 2021
@fedgiac
Copy link
Contributor Author

fedgiac commented Feb 23, 2021

Pull request must be merged manually.
GitHub App like Mergify are not allowed to merge pull request where .github/workflows is changed.

This is why I'm merging manually, all other checks are otherwise successful.

@fedgiac fedgiac merged commit 6cb3e23 into main Feb 23, 2021
@fedgiac fedgiac deleted the coverage branch February 23, 2021 09:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants