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

Sonarcloud with code coverage #2968

Merged
merged 3 commits into from
May 30, 2024

Conversation

jamshale
Copy link
Contributor

@jamshale jamshale commented May 24, 2024

This is used to get code coverage for our sonarcloud integration on PR's and when code is pushed to main.

The complication comes with using the sonarcloud token from forked repos. I have tested using two github accounts and forking my fork.

This works by saving the test report from the PR Tests workflow as an artifact and also the pr number. Then when the PR Tests workflow completes successfully a workflow will begin for sonarcloud witch gets information about the forked repo, and fetches the code for the scan. The code coverage is downloaded from the artifact, updated and then upload to sonarcloud.

Another workflow runs the tests on merge to main, runs the scan and uploads results. I thought about doing this as one workflow but decided separate workflows was actually much simpler.

I changed the workflow Tests to an action and adjusted the other workflows that used it. When calling from the push to main workflow it was having trouble as a workflow.

Requires disabling Automatic Analysis in the sonarcloud admin console. You can't use automatic and manage it from CI at the same time. Also won't exist until the workflows have been merged to main.

Main branch:

image

PR (Failing):

The required coverage can be adjusted in sonarcloud console.

image

image

image

PR (Passing):

image

README.md Show resolved Hide resolved
@jamshale jamshale requested review from swcurran and WadeBarnes May 24, 2024 20:00
@jamshale jamshale marked this pull request as ready for review May 24, 2024 20:02
@jamshale
Copy link
Contributor Author

jamshale commented May 24, 2024

Hmm. Might have messed something up with that Tests / Test Python 3.9 Expected — Waiting for status to be reported

I think it could just be changed to PR Tests, or I could change PR Tests to take an input. I think Tests workflow should still be an action instead of a workflow.

Copy link
Contributor

@WadeBarnes WadeBarnes left a comment

Choose a reason for hiding this comment

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

A few questions, and a suggestion.

Is it necessary to run the sonar scan on the PR and on the merge? Just wondering what the difference is and whether running the scan on merge would be redundant since it's already been run on the PR.

.github/workflows/nigthly.yml Show resolved Hide resolved
.github/workflows/pr-tests.yml Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
@WadeBarnes
Copy link
Contributor

We'll need to update the Required checks to switch to the updated test flow/action. If we do that before we merge this PR, it will have to be done right before the merge, as the change will impact the required checks on all outstanding PRs, and they will need to be updated so they run and pass the new test flow/action.

@jamshale
Copy link
Contributor Author

A few questions, and a suggestion.

Is it necessary to run the sonar scan on the PR and on the merge? Just wondering what the difference is and whether running the scan on merge would be redundant since it's already been run on the PR.

I think it's necessary to get the code coverage on main and not just the PR. Also, I think sonarcloud is already doing a scan on both currently, when using automatic analysis. I can have a look and see if I can find any information on this.

@jamshale
Copy link
Contributor Author

I may be able to use the same code coverage artifact to avoid needing to run the tests on merge to main. I will look at doing that.

For the extra scan on merge to main I don't think anything needs to be changed. This is the same behaviour as automatic analysis after reading the sonarcloud documentation.

@WadeBarnes
Copy link
Contributor

I may be able to use the same code coverage artifact to avoid needing to run the tests on merge to main. I will look at doing that.

For the extra scan on merge to main I don't think anything needs to be changed. This is the same behaviour as automatic analysis after reading the sonarcloud documentation.

Sounds good. Let me know when your ready for final review.

@jamshale jamshale force-pushed the feat/2390 branch 2 times, most recently from 02de8ea to 25a698e Compare May 29, 2024 19:51
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@jamshale
Copy link
Contributor Author

@WadeBarnes So, I tried to use the previous test coverage artifact but have been having a hard time locating it from this workflow. It's a lot different than the other workflow which has the workflow run id. I thought I could use the pr commit, but haven't been able to make it work.

It would be nice to not run the tests twice for the same code, but I also don't want to mess around with this for a long time. So, I think this should be ready and maybe I'll be able to figure it out for a later PR.

@WadeBarnes
Copy link
Contributor

Branch protection checks have been updated to account for the changes made to the workflows in this PR. This will impact ALL existing PRs, which will have to be updated to run the updated workflows in order to pass the branch protection checks.

cc @jamshale, @swcurran, @dbluhm

@WadeBarnes WadeBarnes merged commit db73534 into openwallet-foundation:main May 30, 2024
8 checks passed
@WadeBarnes
Copy link
Contributor

Failure here was my fault. I forgot to turn off automatic analysis. It's off now.

@WadeBarnes
Copy link
Contributor

Job was rerun successfully.

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