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

Fix coverage workflow #4146

Merged
merged 4 commits into from
Jun 6, 2024
Merged

Fix coverage workflow #4146

merged 4 commits into from
Jun 6, 2024

Conversation

kratman
Copy link
Contributor

@kratman kratman commented Jun 6, 2024

Description

Coverage was not working properly, this should fix it

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Key checklist:

  • No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
  • All tests pass: $ python run-tests.py --all (or $ nox -s tests)
  • The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@kratman kratman self-assigned this Jun 6, 2024
Copy link
Member

@agriyakhetarpal agriyakhetarpal left a comment

Choose a reason for hiding this comment

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

Suggestion: we should move the coverage upload step above the integration tests, since the coverage is just for the unit tests anyway – CodeCov will then analyse the coverage and return its results earlier

.github/workflows/test_on_push.yml Outdated Show resolved Hide resolved
@agriyakhetarpal
Copy link
Member

The notebook failure is sporadic and unrelated, should be fixed with a rerun

@agriyakhetarpal
Copy link
Member

Another proposition: the macos-14 images are much faster than the ubuntu-latest runners (~9 minutes vs ~25 minutes), so PyBOP had switched to using them for coverage – maybe we can try that as well? As far as I know, there is just one LaTeX test that needs to run on Linux, we can skip it on macOS to get faster coverage.

@kratman
Copy link
Contributor Author

kratman commented Jun 6, 2024

Another proposition: the macos-14 images are much faster than the ubuntu-latest runners (~9 minutes vs ~25 minutes), so PyBOP had switched to using them for coverage – maybe we can try that as well? As far as I know, there is just one LaTeX test that needs to run on Linux, we can skip it on macOS to get faster coverage.

For now, lets leave it. Especially if it requires skipping some coverage tests

@kratman kratman marked this pull request as ready for review June 6, 2024 17:14
@agriyakhetarpal
Copy link
Member

agriyakhetarpal commented Jun 6, 2024

The coverage upload failed silently again, maybe we need to supply GHA's special ${{ secrets.GITHUB_TOKEN }} with it in an input for it to work?

@kratman
Copy link
Contributor Author

kratman commented Jun 6, 2024

error - 2024-06-06 16:52:39,048 -- Upload failed: {"detail":"Tokenless has reached GitHub rate limit. Please upload using a token: https://docs.codecov.com/docs/adding-the-codecov-token. Expected available in 103 seconds."}

@agriyakhetarpal
Copy link
Member

I think someone with admin permissions will have to add a secret to the repository, then – tagging @pybamm-team/maintainers

@kratman
Copy link
Contributor Author

kratman commented Jun 6, 2024

I think someone with admin permissions will have to add a secret to the repository

codecov/codecov-action#1471

I think it an issue with codecov because I am using a fork for my branch

Copy link

codecov bot commented Jun 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.55%. Comparing base (f3f7b4c) to head (68bfa6e).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #4146   +/-   ##
========================================
  Coverage    99.55%   99.55%           
========================================
  Files          288      288           
  Lines        21790    21790           
========================================
  Hits         21693    21693           
  Misses          97       97           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kratman
Copy link
Contributor Author

kratman commented Jun 6, 2024

Works now, sporadic failures will happen until the codecov fixes a bug

@kratman kratman merged commit b4f8380 into pybamm-team:develop Jun 6, 2024
26 checks passed
@kratman kratman deleted the fix/covReport branch June 6, 2024 18:22
js1tr3 pushed a commit to js1tr3/PyBaMM that referenced this pull request Aug 12, 2024
* Remove quotes from the if-statement

* Adjust logic

* Fix job naming

* Change order
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.

None yet

2 participants