[BUILD-1030] New look at tooling for assessing quality of PRs Created: 10/Mar/23 Updated: 28/Mar/23 Resolved: 28/Mar/23 |
|
| Status: | Closed |
| Project: | Build |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Task | Priority: | Neutral |
| Reporter: | Maxime Michel | Assignee: | Maxime Michel |
| Resolution: | Done | Votes: | 1 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Attachments: |
|
||||||||||||
| Issue Links: |
|
||||||||||||
| Template: |
|
||||||||||||
| Acceptance criteria: |
Empty
|
||||||||||||
| Task DoR: |
Empty
|
||||||||||||
| Team: | |||||||||||||
| Description |
|
As we nowdays automatically deploy to the SaaS anything we merge to master, we need to better check the quality of the code. We can't enforce something like a JaCoCo threshold due to amount of untested code. What we're looking for instead is PR-level validation. Here are a couple of options: bitbucket-code-coverageThe easiest to roll out solution I've seen. Using JaCoCo reports as material, it annotates the code to highlight tested/untested lines of code. This only requires a Bitbucket server plugin and running the following commands:
mvn clean verify -Penable-code-coverage jacoco:report
mvn com.atlassian.bitbucket:code-coverage-maven-plugin:publish \
-Dbitbucket.url=https://git.magnolia-cms.com \
-Dbitbucket.user=mmichel \
-Dbitbucket.password=$LDAP_PSWD \
-Dbitbucket.commit.id=8e3839923cb1434dcf320e1c9115676d206ef081 \
-Dcoverage.file=magnolia-core/target/site/jacoco/jacoco.xml \
-Dcoverage.format=JACOCO
Source: https://bitbucket.org/atlassian/bitbucket-code-coverage/src/master/ Problem: this isn't helpful in the sense that it doesn't validate the PR with a yes/no answer to the question of whether the quality is sufficient. As agreed to in the March 21st' architecture meeting, it's anyway another tool in the toolbox, we might as well give it a try. Bitbucket code insightsAlready built into Bitbucket is an API to which we can post checks of any nature about the code: https://confluence.atlassian.com/bitbucketserver/code-insights-966660485.html This is (I assume) what is used by professional tools such as Sonarqube. However, because it's a REST API, nothing prevents us from pushing simpler reports/metrics into it: test coverage, Snyk report, style, etc. The downside is that it will require inhouse development work & maintenance. It will also never match Sonarqube's own insights. jacoco-report GitHub actionUsing the Jacoco data and a bit of tinkering, as well as the Bitbucket code insights, it is possible to calculate the coverage for the PR's changed files. This coverage can then be compared to a limit, and fail the PR if deemed insufficient. See https://github.com/Madrapps/jacoco-report as well as attached screenshot for Bitbucket Server rendering curl -u mmichel:$LDAP_PSWD --request PUT 'https://git.magnolia-cms.com/rest/insights/latest/projects/PLATFORM/repos/main/commits/8e3839923cb1434dcf320e1c9115676d206ef081/reports/test-report-1' \ --header 'Content-Type: application/json' \ --data-raw '{ "title": "Coverage PR quality check", "details": "This checks evaluates whether coverage on changed files is sufficient.", "result": "FAIL", "data": [ { "title": "Number of analysed files", "type": "NUMBER", "value": 1 }, { "title": "Coverage on changed files", "type": "PERCENTAGE", "value": 80 }, { "title": "Safe to merge?", "type": "BOOLEAN", "value": false } ], "reporter": "Foundation" }' SonarqubeWe have already looked into rolling Sonarqube. It's likely a popular name on the market for a good reason, however, we've found it expensive per line of code. Also, it will need to be adapted to our code style.
Conclusion: we roll out SonarQube. |
| Comments |
| Comment by Maxime Michel [ 22/Mar/23 ] |
|
Just added two screenshots to show another type of check running on some PRs that I found out by chance. |
| Comment by Maxime Michel [ 28/Mar/23 ] |
|
Just added a screenshot of the type of enforcing Sonar will do on PRs. It's effectively enforcing coverage (without us having to do the implementation described above) but at the same time also evaluating security, technical debt, etc. It's great! |
| Comment by Maxime Michel [ 28/Mar/23 ] |
|
Going with SonarQube makes the most sense. See linked tickets for progress. |