-
Notifications
You must be signed in to change notification settings - Fork 53
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
chore: add sonarcloud reporting for aggregate test coverage in gax, api-common and showcase #1482
Conversation
|
Showcase integration tests expect the showcase server to be running |
@@ -1,4 +1,6 @@ | |||
name: SonarCloud Build | |||
env: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have showcase version here and in ci-maven.yaml as well, I'm not very familiar with Github actions, is it possible to configure a shared SHOWCASE_VERSION
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is! According to the docs, shared variables can be created by doing Settings > secrets and variables > Actions > Manage environments
. However, I'm not able to see the Settings
tab for this repo so I think it requires some special permissions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are going to have hermetic builds, the version needs to be stored in source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thank you. Managing the variables in Settings may not be the best solution in that case. I'm inclined to keep the duplicate showcase_versions until an alternate solution becomes available in favor of making the builds self-contained and maintaining visibility into the showcase version we are testing against. Open to hearing more thoughts on this though.
api-common-java/pom.xml
Outdated
@@ -131,4 +131,19 @@ | |||
</plugin> | |||
</plugins> | |||
</build> | |||
|
|||
<!-- Skip api-common when analyzing showcase test coverage on SonarCloud --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we skip api-common
? It's also a handwritten runtime dependency of client libraries, same as gax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the test coverage of showcase is restricted to the showcase and gax modules because of how heavily dependent GAPIC clients are on gax (which contains a lot of the transport logic). In terms of development, a feature or a bug fix oftentimes has a corresponding change in gax.
However, an argument can be made in support of api-common being included. The coverage report for showcase is to provide insights into how much of the core code the showcase tests are exercising, observe coverage drop/rise as we continue developing and also help us determine if it is safe to replace the existing handwritten integration tests. Given this purpose, do we think having a combined test coverage of api-common
, gax-java
and showcase
tells a more complete story?
cc/ @burkedavison
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me. If it's part of the 'platform runtime', let's include it. In the future this may also include java-core if we have a handwritten layer on top of showcase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! Will look into adding this in as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, updated the README and PR description as well.
…r-java into sonar-coverage
[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
[java_showcase_integration_tests] Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
[java_showcase_unit_tests] Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Follow up to #1430
Notable Changes
sonar.coverage.jacoco.xmlReportPaths
to the jacoco xml file created incoverage-report
.sonar.skip
to the modules that need to be excluded from showing up in the showcase reporting projects on Sonarcloud. These includejava-iam
,gapic-generator-java
, etc.Sample illustration of results
The result will show up in the following format on the
main
branch where a separate project has been created for each type of analysis (one for showcase unit test analysis and another for showcase integration test analysis).The images below are provided for illustrative purposes and display results from an experiment conducted under a test Organization for a forked
gapic-generator-java
repo (https://sonarcloud.io/organizations/mpeddada1/projects?sort=-name)Example of
showcase_integration_tests
coverage from tests with forked repo. :Note of caution when comparing Jacoco vs SonarCloud: There is a slight discrepancy between the %s reported in SonarCloud vs Jacoco which we've used to gather the coverage data. For example, for integration test reporting, while the jacoco report in #1430 shows 15% branch coverage, SonarCloud in the test conducted shows 17.5% for conditional coverage. This could be attributed to how each of the tools interpret and calculate each coverage type. Reference: https://community.sonarsource.com/t/sonarqube-and-code-coverage/4725#h-1-to-1-comparison-between-sonarqube-and-coverage-report-is-not-relevant-3 and https://stackoverflow.com/a/40342671/14357112