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

trying to fix coverage #735

Merged
merged 8 commits into from
Oct 18, 2024
Merged

trying to fix coverage #735

merged 8 commits into from
Oct 18, 2024

Conversation

pavelkomarov
Copy link
Contributor

@pavelkomarov pavelkomarov commented Oct 7, 2024

The coverage badge is broken in the readme, and it looks like the last time coverage was successfully used was 2019! I'm attempting a fix by following an old example of mine where I figured this out. I'll need one of you repo maintainers to sign in to the coverage account and generate a token and put it as a secret in this repo, or go look at the repo secrets and tell me what the token is named. (It may already exist.)

@mandli
Copy link
Member

mandli commented Oct 7, 2024

I think I just added it...?

@pavelkomarov
Copy link
Contributor Author

pavelkomarov commented Oct 7, 2024

Thanks! In the repo's Settings, there's a Security section, and it goes in there. It should be named COVERALLS_REPO_TOKEN to work with the specific commit I just made, but you can name it whatever, and I can update the script to agree.

Screenshot 2024-10-07 at 3 28 03 PM

If all is right, then rerunning the Action should result in a push to coveralls.io (and some useful stuff there).

@mandli
Copy link
Member

mandli commented Oct 7, 2024

I called it COVERALLS_REPO_TOKEN and added it there so if that does not work we will have to see if I used the right token.

@pavelkomarov
Copy link
Contributor Author

I don't think I have the power to rerun the job, and I'd rather not make a dummy commit. You'll have to test it out.

@mandli
Copy link
Member

mandli commented Oct 8, 2024

I cannot tell if anything worked but I think we should separate out the coverage test from the primary test to make things clear.

@ketch
Copy link
Member

ketch commented Oct 8, 2024

I'm not sure how to test this. I ran the same commands locally and I get

Error running coveralls: Not on TravisCI. You have to provide either repo_token in .coveralls.yml or set the COVERALLS_REPO_TOKEN env var.

Sorry, I haven't used coveralls in a long time and didn't take the time to look up what to do. @mandli the point of this is to report what fraction of the code is being tested. So in a sense they can't be separated. Or do you mean to run all the tests twice?

@mandli
Copy link
Member

mandli commented Oct 8, 2024

You might be able to add the COVERALLS_REPO_TOKEN as an environment variable but it is added correctly as a secret as far as I can tell. I also was mistaken about separating, I thought it could be made more independent of the tests than we can.

@pavelkomarov
Copy link
Contributor Author

So when the build runs, we should end up with info here https://coveralls.io/github/clawpack/pyclaw. If you go under the Actions tab for the repo and rerun the latest build, then hopefully it succeeds at pushing information to coveralls at the end. It failed to do so when I opened this PR (thus kicking off a CI job), because the COVERALLS_REPO_TOKEN didn't yet exist. I don't have the power to rerun that job directly from the Actions, but you probably do. I could push some other commit to this PR to start another job, but I'd rather not create an extra commit.

@mandli
Copy link
Member

mandli commented Oct 14, 2024

I did restart the action but it just failed out again. I am guessing that it isn't pushing the information to coveralls because of the failure but that sort of does not make sense to me given that you would probably want to have coverage info regardless of the tests failing.

@pavelkomarov
Copy link
Contributor Author

Hmm. The tests are run with a coverage call, so the coverage info should be generated, and then the next call in the CI script is to upload that info. Does the build choose to fail before that next call is made? I think it makes some sense to not have coverage info for broken builds cluttering the coverage history. The assumption is the build shouldn't be broken most of the time.

@mandli
Copy link
Member

mandli commented Oct 14, 2024

I just restarted it again with debug logging turned on (not sure what that actually does TBH). coveralls is called after the pytest call so that might be why. We may want to modify the pytest call to not stop at a particular failure as well.

@pavelkomarov
Copy link
Contributor Author

pavelkomarov commented Oct 14, 2024

I don't think there's a way to call coveralls except for after the tests are done. First the coverage info is accumulated during tests, then it's uploaded. If you indeed would like coverage uploaded regardless of failure status, is there a way to bypass the failure? I'm having trouble figuring out whether coveralls is even meant to do coverage for failing builds. Here's what Google's AI says:
Screenshot 2024-10-14 at 3 05 16 PM

@mandli
Copy link
Member

mandli commented Oct 14, 2024

Looks like we just need to fix that test.

@pavelkomarov
Copy link
Contributor Author

pavelkomarov commented Oct 16, 2024

Okay, I spent a while playing with the yml script. If I make the coveralls call its own task and invoke always(), then I can get it to run even when tests fail, and it comes through in the coverage history 🎉 But hold your celebration, because the status is "pending completion", which I take to mean coverage didn't finish running, so there isn't really a report. Somebody else asked about forcing a report even in the presence of failing tests and didn't get any answers.

@pavelkomarov
Copy link
Contributor Author

I suspect the fact the test is failing with a segfault may make wrapping up the coverage report less likely. If I run locally, one of the examples/advection_1d_variable/test_variable_coefficient_advection.py ....F tests fails, but the shallow water tests don't segfault. In this case, I do get a .coverage file output.

@pavelkomarov
Copy link
Contributor Author

Screenshot 2024-10-16 at 5 00 28 PM

Ahha. There is indeed no .coverage file created, likely due to the segfault.

@ketch
Copy link
Member

ketch commented Oct 17, 2024

@pavelkomarov If you rebase this onto main (now that I merged #736) it should avoid the segfault.

@pavelkomarov
Copy link
Contributor Author

pavelkomarov commented Oct 17, 2024

Okay, something curious now: The tests finish, but .coverage isn't where I expected. I think I have to re-cd down to the right directory. So maybe it was actually there even with segfault? Unclear. Trying again.
Screenshot 2024-10-17 at 11 42 33 AM

@pavelkomarov
Copy link
Contributor Author

Okay! The job finished, and it just showed "pending completion" again.
Screenshot 2024-10-17 at 11 53 55 AM
But then after giving it a second, it updated.
Screenshot 2024-10-17 at 11 55 15 AM

Back in business, boys!

@ketch
Copy link
Member

ketch commented Oct 18, 2024

Thank you and nice work, @pavelkomarov !

@ketch ketch merged commit 37658ee into clawpack:master Oct 18, 2024
2 checks passed
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.

3 participants