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

test: add language starter module back to sonar analysis #1816

Merged
merged 10 commits into from
May 15, 2023

Conversation

emmileaf
Copy link
Contributor

@emmileaf emmileaf commented May 8, 2023

This PR update sonar exclusion on spring-cloud-previews so that analysis runs on google-cloud-language-spring-starter, which contain the handwritten unit tests. This allows for some indication of missing coverage should there be broader configuration code changes introduced by the generator.

  • Also updates the tests so that existing code coverage is sufficient (previously missing for some method-level retry property setters)

@emmileaf emmileaf marked this pull request as ready for review May 8, 2023 21:56
@emmileaf
Copy link
Contributor Author

emmileaf commented May 8, 2023

Note: the final sonarcloud comment will show "no coverage info" on this PR because it does not make any changes to the autoconfiguration code. With the mock changes in commit b7acf97, this analysis looks like below, catching changes to the google-cloud-language-spring-starter module and indicating missing coverage on new lines:

image

@sonarcloud
Copy link

sonarcloud bot commented May 8, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@meltsufin meltsufin left a comment

Choose a reason for hiding this comment

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

LGTM.
PS: I didn't realize we were mixing auto-generated code with handwritten, although it's just tests. I'm curious @blakeli0's opinion on this. Is this a testing strategy we want to employ?

Copy link
Contributor

@zhumin8 zhumin8 left a comment

Choose a reason for hiding this comment

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

LGTM.
About test strategy, we created manual written unit test for one sample generated module and it is "mixed-in" as copied from template folder to generated code to run test with any updates to the generated module. It's the simplest setup to have coverage on the generated code.
Alternative could be to copy generated "language-starter" module after generated to a separate un-published module where these tests locates and do testing there. What do you all think? @emmileaf @meltsufin @blakeli0 Is it worth the change so we can keep generated code modules clean from manual code?

@meltsufin
Copy link
Member

LGTM. About test strategy, we created manual written unit test for one sample generated module and it is "mixed-in" as copied from template folder to generated code to run test with any updates to the generated module. It's the simplest setup to have coverage on the generated code. Alternative could be to copy generated "language-starter" module after generated to a separate un-published module where these tests locates and do testing there. What do you all think? @emmileaf @meltsufin @blakeli0 Is it worth the change so we can keep generated code modules clean from manual code?

I think that the purpose of the tests is to test the Spring starter generator. So, I wonder if you considered generating a starter for Showcase (or another API) and writing manual tests against it, all inside the spring-cloud-generator module?

@emmileaf
Copy link
Contributor Author

Thanks for the reviews @meltsufin @zhumin8 !

I think that the purpose of the tests is to test the Spring starter generator. So, I wonder if you considered generating a starter for Showcase (or another API) and writing manual tests against it, all inside the spring-cloud-generator module?

1355 has some context from when these handwritten unit tests were first added - I recall the initial reasoning for not using showcase being 1) it required additional setup, and 2) showcase module provides power of coverage for client library functionalities, whereas the generated code are just an integration layer on top of these.

The purpose of these handwritten unit tests is to have some test coverage for generated code, alongside the golden tests we have for the generator itself.

Alternative could be to copy generated "language-starter" module after generated to a separate un-published module where these tests locates and do testing there.

Regarding where these tests should live, whether alongside the generated code or in a separate module: copying the generated module and running this test elsewhere is definitely an option, and I also wonder if there is a way to relocate the test code but still run it on the generated module. With the plan to add a manual integration test layer (auth, several services working together), having a separate module for handwritten tests does makes sense to me.

I’ll go ahead and merge this PR for now, but will follow-up with exploring/revisiting some of these options. (Happy to continue the discussion in this thread as well)

@emmileaf emmileaf merged commit 5347d40 into main May 15, 2023
28 checks passed
@emmileaf emmileaf deleted the codegen-sonar-exclusions branch May 15, 2023 17:53
emmileaf added a commit that referenced this pull request May 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants