-
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
Changes from 41 commits
d7c919a
135022c
d2be02f
22cda89
cd0de15
cddb9f2
c116da1
208cfff
02ef0cb
7f720b8
68557fe
57ddf93
fcc5257
45a0987
47cf349
e61d6ea
83f085d
d4ba320
b856e7f
f574ec8
61bb283
6c54fbe
644edc8
afcf7b1
cff1fa4
b9db44f
75a51ec
6972c9f
7aab6ca
34869ff
0d342c8
5aa2013
36d1633
6f7ead4
63cec27
efc6ecc
9d676c0
03fd1b9
74fbf20
7e4202a
6264511
5a662a1
1ab860b
bcd8299
27afb2e
4b60a61
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Why do we skip There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 cc/ @burkedavison There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Done, updated the README and PR description as well. |
||
<profiles> | ||
<profile> | ||
<id>showcase-sonar-analysis</id> | ||
<activation> | ||
<property> | ||
<name>enableTestCoverage</name> | ||
</property> | ||
</activation> | ||
<properties> | ||
<sonar.skip>true</sonar.skip> | ||
</properties> | ||
</profile> | ||
</profiles> | ||
</project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -177,7 +177,6 @@ | |
</build> | ||
</profile> | ||
</profiles> | ||
|
||
<repositories> | ||
<repository> | ||
<id>google-maven-central-copy</id> | ||
|
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 theSettings
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.