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 code coverage metric into CI #262

Merged
merged 7 commits into from
May 22, 2024

Conversation

maxloeffler
Copy link
Contributor

@maxloeffler maxloeffler commented May 6, 2024

Prerequisites

  • I adhere to the coding conventions (described here) in my code.
  • I have updated the copyright headers of the files I have modified.
  • I have written appropriate commit messages, i.e., I have recorded the goal, the need, the needed changes, and the location of my code modifications for each commit. This includes also, e.g., referencing to relevant issues.
  • I have put signed-off tags in all commits.
  • I have updated the changelog file NEWS.md appropriately.
  • I have checked whether I need to adjust the showcase file showcase.R with respect to my changes.
  • The pull request is opened against the branch dev.

Description

Add codecov coverage to CI. Steps to make it run for the main repo:

  1. Go to codecov.io and connect the GitHub account of a repo-admin of coronet to the service.
  2. "Activate" codecov by hitting the activate button on their website.
  3. Obtain the repo specific "Repository upload token".
  4. Enter the GitHub repo settings for coronet, go to Security -> Secrets and variables -> Actions -> Repository secrets and add token there with key CODECOV_TOKEN. GitHub will then automatically and secretly supply token to coverage.R when executing the CI.
  5. Install the codecov GitHub App and allow it access to the repo.

Changelog

  • Introduce coverage.R that generates coverage reports
  • Adjust CI workflow to accommodate for automatic coverage report generation

maxloeffler and others added 2 commits May 6, 2024 15:38
Signed-off-by: Maximilian Löffler <s8maloef@stud.uni-saarland.de>
As the recent package version of Matrix is not compatible any more with
R versions < 4.4.0, but the Matrix package version that is automatically
shipped with R 3.6 is smaller than the minimum package version of Matrix
that coronet requires. To circumvent this problem, specifically install
Matrix version 1.3-4 from the package archives if the minimum version
required by coronet is not met. This change should not affect recent R
versions, which automatically ship a sufficient version of Matrix.

Signed-off-by: Thomas Bock <bockthom@cs.uni-saarland.de>
@maxloeffler
Copy link
Contributor Author

Currently, I had to comment out one broken test. You can review that one and we can discuss about possible solutions to it 😄.

@bockthom
Copy link
Collaborator

bockthom commented May 6, 2024

Does the error at the end of the coverage generation occur because of the missing token in our repo configuration, or because of any other error?

@maxloeffler
Copy link
Contributor Author

Because of the missing token. At the end of the report we see a warning which we can disregard, then followed by 2 lines of trying to access codecov's API, which fails because a lack of token 🤓

@bockthom
Copy link
Collaborator

bockthom commented May 6, 2024

Ok, thanks. Then I will enter the token (as you have described above in the Description of this PR) later and re-run the failing build thereafter.

@bockthom
Copy link
Collaborator

bockthom commented May 6, 2024

@MaLoefUDS Sorry for triggering a series of failing builds. I followed your description, but unfortunately I had added the token to the wrong repository. That's the reason why it did not work in my last two tries. Now I've added the token to the correct repository prior to the current run. So, I expect the current run to succeed. If it does not succeed, then there might be something else going wrong... Let's keep our fingers crossed!

@bockthom
Copy link
Collaborator

bockthom commented May 6, 2024

Hm, still not working. I don't know what to do now... Maybe we can have a closer look at your settings in your fork during our next meeting to find out what's going wrong here?

@bockthom
Copy link
Collaborator

bockthom commented May 8, 2024

Activating the repo on codecov was not enough 😞
However, I found out that there is a link "learn more" on the settings page of codecov - this leads to an outdated documentation page of codecov (the screenshots are outdated), but the outdated page contains a link "app configuration". Using this link, I was forwarded to a GitHub page, were I could request to install codecov for se-sic/coronet. Although the page says that the request was successful, an administrator of our se-sic organization received an e-mail that he should approve my request. After his approval, codecov appears in the list of installed apps for coronet. So, let's hope that it will work in the current CI run...

@maxloeffler
Copy link
Contributor Author

Ohhh yes I remember that I did something like that as well 😅 Sorry that I forgot to list it

@bockthom
Copy link
Collaborator

bockthom commented May 8, 2024

Still failing... 😞

I have an idea why this might not work yet:

The default branch is master. Even when I go to the codecov settings to change the default branch, the drop-down menu does not show me any other branch than master. Thus, codecov even has not crawled the available branches yet 😞

So, the next question is: Does this work when the PR is not merged yet? As far as I can see, the PR does not contain any file that needs to be accessed by codecov, so everything should be present in GitHub's and codecov's settings and nothing needs to be present in the repository, right?

And the other question is: Does this only work after it has been merged to the default branch (i.e., master) or can we tweak some settings such that it will connect to codecov right away? Or do we need to specifiy the GitHub action somewhere, as indicated in the general page of codecov?

@bockthom
Copy link
Collaborator

bockthom commented May 8, 2024

It could also be that our problem is related to this issue: codecov/feedback#112
It's quite a long discussion there and I didn't have enough time to read the whole conversation, but initially they said that PRs from forks cannot access the CODECOV_TOKEN as this is a secret from the upstream repo and that they have been working on a solution for that. During their conversation, they talk about lots of workarounds, but I gave up on reading all the posts up until they proposed an acutual solution.
@MaLoefUDS: Maybe you can have a look at the conversation there, especially the most recent parts of it? Maybe they have already implemented a solution that also works for us?


Independent of the issue linked above: May it be an idea to install xml2 inside the container, as suggested by the error message at the end of your pipeline? I am wondering why we don't get a proper error message, but maybe that's just because of R things...


Any other ideas on what we can do?

@maxloeffler
Copy link
Contributor Author

After reading the linked issue, I might understand why it fails for us. I also believe that fixing that might require some more work 🫠.

Possible reason for the fail: Of course it is a privacy leak when GitHub would give the repo secrets out when a workflow is triggered from a forked repo. Everyone can create a fork and shadow a file that usually handles the secret to leak / steal it. Therefore what I think happens is that GitHub just nulls the secret value for workflows triggered from forks. With no secret, the coverage.R file cannot connect properly to the codecov API.

Possible fixes: Codecov seem to have thought of this issue and implemented "tokenless" authentication for workflows triggerd from forks when using codecov/codecov-action@v4 in a workflow (read about it here). Currently, I was not using codecovs GitHub action. I tried making it work once, failed, and just moved on the current solution (that is, just passing the secret as a CLI arg to coverage.R). So currently I see 2 options: 1) Have code coverage only update with PR merge. I don't like this approach but it is possible. 2) Make the GitHub action work 🤓. I guess you have similar thoughts so I will just start and get to work 😅

This should make the coverage-report uploading also possible inside of
PRs that originate from forks of the main coronet repo (see, PR se-sic#262).

Signed-off-by: Maximilian Löffler <s8maloef@stud.uni-saarland.de>
@maxloeffler
Copy link
Contributor Author

Easier done than said 💪! I oriented myself at the basic example file codecov provides! Now I think the only thing missing is the adjustment of the badge.

Copy link

codecov bot commented May 13, 2024

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@bockthom
Copy link
Collaborator

Easier done than said 💪! I oriented myself at the basic example file codecov provides!

Great!

Now I think the only thing missing is the adjustment of the badge.

Here is the code for the current badge:

[![codecov](https://codecov.io/gh/se-sic/coronet/graph/badge.svg?token=2dhAb3ScLy)](https://codecov.io/gh/se-sic/coronet)

However, I am not sure which branch we should choose for the badge. Default refers to master I guess. Not sure whether we should use master or dev. Any ideas on that?

Anyway, independent of which branch we will choose for the badge, we should add the branch name after test coverage.

Now I think the only thing missing is the adjustment of the badge.

And also the tests that you disabled due to mismatches in path names should be fixed, I guess...

@maxloeffler
Copy link
Contributor Author

About the batches: We can trivially obtain badges for both branches, once there has been at least one run of the new CI on both branches. The branch is encoded in the link (once you have uploaded runs from multiple branches), so we could even install the badge for master before even running the CI once then just leading to a broken link:

https://codecov.io/gh/MaLoefUDS/coronet/branch/test/graph/badge.svg?token=OU3SR362X9
codecov
https://codecov.io/gh/MaLoefUDS/coronet/branch/dev/graph/badge.svg?token=OU3SR362X9
codecov
https://codecov.io/gh/MaLoefUDS/coronet/branch/master/graph/badge.svg?token=OU3SR362X9 (non-existent)
codecov

So I would just add both badges after the respective build status badges.

About the missing test: Yes, you're right, I forgor 💀

Signed-off-by: Maximilian Löffler <s8maleof@stud.uni-saarland.de>
Introduce a new constant in 'testing-utils.R' that describes a prefix
of the path leading to the testing data files. This constant is
necessary since the 'coverage.R' run of all tests demands a different
path prefix than the 'tests.R' run of all tests.

Signed-off-by: Maximilian Löffler <s8maloef@stud.uni-saarland.de>
@maxloeffler
Copy link
Contributor Author

maxloeffler commented May 13, 2024

(The failure right now is not a failure in the classical sense. It is a known issue described in the link on the tokenless authentication of codecov I sent above. I don‘t know to which extend we can try to fix that issue, but we can atleast make the CI not fail if the uploading fails if you’re fine with that)

@bockthom
Copy link
Collaborator

(The failure right now is not a failure in the classical sense. It is a known issue described in the link on the tokenless authentication of codecov I sent above. I don‘t know to which extend we can try to fix that issue, but we can atleast make the CI not fail if the uploading fails if you’re fine with that)

Hm, that sounds like that this would appear comparably often. If we always push in the "wrong moments", we'd never receive a coverage report. And failing checks are also bad if everything is ok except for that rate limit stuff of codecov.
According to issue codecov/feedback#301, the codecov guys are currently working on it -- if I understand it correctly, they reported 4 weeks ago that they will come up with a solution in about 4 weeks... Not sure if it does make sense to wait for it or not...

@maxloeffler
Copy link
Contributor Author

Hmm, maybe we can have a while loop in the Github action that sends the request until it went through for the time where they did not implement a fix? 🤔 Appart from that I'm not sure what else we can even do

bockthom
bockthom previously approved these changes May 17, 2024
Copy link
Collaborator

@bockthom bockthom left a comment

Choose a reason for hiding this comment

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

This PR looks good to me. 👍

@MaLoefUDS Should we already merge this PR or did you find an easy possibility to separate uploading the coverage report?

@maxloeffler
Copy link
Contributor Author

I have made some research. GitHub provides some form a persistence called Artifacts. I was not able to fully explore that, but I actually believe that it could work for us

This allows us to rerun only the upload if it fails because of rate limiting.

Signed-off-by: Maximilian Löffler <s8maloef@stud.uni-saarland.de>
Signed-off-by: Maximilian Löffler <s8maloef@stud.uni-saarland.de>
@bockthom bockthom merged commit 4d35daa into se-sic:dev May 22, 2024
9 checks passed
@maxloeffler maxloeffler deleted the wip-coverage-reports branch November 12, 2024 13:46
Leo-Send pushed a commit to Leo-Send/coronet that referenced this pull request Nov 13, 2024
This should make the coverage-report uploading also possible inside of
PRs that originate from forks of the main coronet repo (see, PR se-sic#262).

Signed-off-by: Maximilian Löffler <s8maloef@stud.uni-saarland.de>
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