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

Integrate test coverage with nile-coverage #494

Merged
merged 20 commits into from
Nov 10, 2022

Conversation

ericnordelo
Copy link
Member

@ericnordelo ericnordelo commented Oct 13, 2022

Fixes #455

PR Checklist

  • configure test coverage check on CI
  • add test coverage badge on README

@ericnordelo ericnordelo marked this pull request as ready for review October 18, 2022 11:58
@ericnordelo
Copy link
Member Author

ericnordelo commented Oct 18, 2022

I made the PR ready for review, just adding the tox coverage environment because only an organization admin can add the integration with codecov.io (badge included).

As it is, this PR allows run coverage using tox -e coverage.

This can be tested locally already after cairo-lang 0.10.1 release.

NOTE: merging this PR will close #455, even when we don't have the integration with CI and the Badge.

Copy link
Contributor

@martriay martriay left a comment

Choose a reason for hiding this comment

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

code looks good! let's wait for @frangio for the setup. will try it out.

tox.ini Show resolved Hide resolved
@@ -29,6 +29,18 @@ extras =
commands =
pytest {posargs}

[testenv:coverage]
description = Invoke nile-coverage to generate coverage report
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add a rule to require PRs to maintain the coverage?

Copy link
Member Author

Choose a reason for hiding this comment

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

I assumed this would be part of the CI action that I have no permission to add (integrating with codecov). I left that part to @frangio (because the solidity contracts repo is also integrated with codecov and this check), but I can work on it if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes this should be handled by Codecov.

@@ -29,6 +29,18 @@ extras =
commands =
pytest {posargs}

[testenv:coverage]
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make sense to run the tests a second time? should we simply run coverage?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think running coverage is enough because the VM used for this is almost the same one that comes default with cairo (the difference, for now, is just storing the touched program counters), so tests are run for coverage in the same environment in practice.

@martriay
Copy link
Contributor

It didn't work for me :(

image

@ericnordelo
Copy link
Member Author

ericnordelo commented Oct 23, 2022

It didn't work for me :(

I forgot to add that this plugin requires python 3.9 (it doesn't work with 3.8 because of these str functions). I will remove these functions and add support for 3.8 tomorrow because it still makes sense in my opinion.

@ericnordelo
Copy link
Member Author

I forgot to add that this plugin requires python 3.9 (it doesn't work with 3.8 because of these str functions). I will remove these functions and add support for 3.8 tomorrow because it still makes sense in my opinion.

This should be fixed in version 0.2.1 (set in last commit).

@frangio
Copy link
Contributor

frangio commented Oct 24, 2022

I've installed the Codecov GitHub App in this repository. I think you still need to set up Step 4 in this guide. (Note that the CODECOV_TOKEN is not necessary because we are using GitHub Actions, this is mentioned in Step 2.)

@codecov
Copy link

codecov bot commented Oct 25, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@c480259). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head ad4061b differs from pull request most recent head 6c66e24. Consider uploading reports for the commit 6c66e24 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #494   +/-   ##
=======================================
  Coverage        ?   96.82%           
=======================================
  Files           ?       28           
  Lines           ?     1419           
  Branches        ?        0           
=======================================
  Hits            ?     1374           
  Misses          ?       45           
  Partials        ?        0           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@frangio
Copy link
Contributor

frangio commented Oct 25, 2022

If you prefer, you can disable the Codecov comment in the config file. See our configuration and the docs.

@ericnordelo
Copy link
Member Author

ericnordelo commented Oct 25, 2022

If you prefer, you can disable the Codecov comment in the config file. See our configuration and the docs.

That config is part of the PR already (.codecov.yml).

Copy link
Contributor

@martriay martriay left a comment

Choose a reason for hiding this comment

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

Left a few questions!

.codecov.yml Outdated Show resolved Hide resolved
.codecov.yml Outdated Show resolved Hide resolved
.codecov.yml Show resolved Hide resolved
.github/workflows/python-app.yml Outdated Show resolved Hide resolved
.github/workflows/python-app.yml Outdated Show resolved Hide resolved
.github/workflows/python-app.yml Outdated Show resolved Hide resolved
tests/token/erc20/ERC20BaseSuite.py Show resolved Hide resolved
tox.ini Show resolved Hide resolved
Co-authored-by: Martín Triay <martriay@gmail.com>
Copy link
Contributor

@martriay martriay left a comment

Choose a reason for hiding this comment

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

Almost there :)

.codecov.yml Outdated Show resolved Hide resolved
.github/workflows/python-app.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/release.yml Show resolved Hide resolved
Co-authored-by: Martín Triay <martriay@gmail.com>
@martriay martriay merged commit e96b64c into OpenZeppelin:main Nov 10, 2022
@ericnordelo ericnordelo deleted the feat/integrate-coverage-#455 branch November 10, 2022 23:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set up test coverage
4 participants