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

Add test coverage #183

Merged
merged 5 commits into from
Jul 17, 2024
Merged

Conversation

rbarman
Copy link
Contributor

@rbarman rbarman commented Jul 14, 2024

Description

Add test coverage (#181)

The .github/workflows/python-tests.yml workflow now:

Also updated CONTRIBUTING.md with steps on how to locally generate the test coverage report.

Checklist

  • Code compiles correctly
  • Created tests which fail without the change (if applicable)
  • All lint and unit tests passing
  • Extended the README / documentation, if necessary

Additional Comments

Repo owners will need to enable this repo with Codecov and set the CODECOV_TOKEN as a repository secret.

@rbarman
Copy link
Contributor Author

rbarman commented Jul 14, 2024

Hi @bgrnwd , the pipeline fails at the "Pytest coverage comment" step when it tries to comment on this PR.

Error: HttpError: Resource not accessible by integration
Error: Resource not accessible by integration

But it is successful on my fork - https://github.com/rbarman/itscalledsoccer/actions/runs/9917117549/job/27400194105

I'm skimming through some other threads like actions/labeler#12 and it looks like the issue is that PRs from forks have read only access to the main repo and thus it can't create the coverage comment.

Might have to do more research, but some possible options that come to mind:

  • Remove the action step that creates a comment. The PR approver will need to go to Codecov to review code coverage.
  • Update this repo settings for forks to have more access. (Not really ideal for security issues)
  • Give people permissions to create branches directly on this repo. (Also not ideal)

bgrnwd
bgrnwd previously approved these changes Jul 17, 2024
@bgrnwd
Copy link
Member

bgrnwd commented Jul 17, 2024

@rbarman Just saw your comment. I think the first option is the way to go for now and we can add a Code Cov badge to the README.

@rbarman
Copy link
Contributor Author

rbarman commented Jul 17, 2024

Alright, I need to create another commit to go with option 1

@bgrnwd bgrnwd merged commit 6f2c13f into American-Soccer-Analysis:main Jul 17, 2024
5 checks passed
@rbarman
Copy link
Contributor Author

rbarman commented Jul 18, 2024

@bgrnwd , thanks for merging. Just as a reminder, you or the repo owners will need to enable this repo with Codecov and set the CODECOV_TOKEN as a repository secret. You go to repo settings -> "Secrets and variables" -> "Actions" -> "New Repository Secret"

Because I just checked the github action runs for this PR (https://github.com/American-Soccer-Analysis/itscalledsoccer/actions/runs/9967409315/job/27541042634#step:7:34) and see:

Error: Codecov token not found. Please provide Codecov token with -t flag.

@bgrnwd
Copy link
Member

bgrnwd commented Jul 20, 2024

Looks like we already have a CODECOV_TOKEN set up from a couple years ago so I'll investigate that error a little more

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.

2 participants