Skip to content

Conversation

@blakeli0
Copy link
Contributor

@blakeli0 blakeli0 commented Jun 5, 2023

No description provided.

@blakeli0 blakeli0 requested a review from a team as a code owner June 5, 2023 17:59
@product-auto-label product-auto-label bot added the size: s Pull request size is small. label Jun 5, 2023
DEVELOPMENT.md Outdated
1. Traditional unit tests
2. Golden unit tests
3. Showcase integration tests
4. Golden integration tests(We should stop adding new ones, and rely on golden unit tests and showcase tests)
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think this makes it sound like there is a possibility/ use case to add in the occasional golden integration tests.

Is there ever a use case/ need to add in new goldens? If not, I think we should just change it from We should to Do not add new ones ... and specify that we want to focus on adding new showcase tests while making the current goldens work.

Or if there is, maybe it would help to list some examples for when we should add goldens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there ever a use case/ need to add in new goldens? If not, I think we should just change it from We should to Do not add new ones ... and specify that we want to focus on adding new showcase tests while making the current goldens work.

Great question and I don't have a definite answer, we need to audit the current golden integration tests and see if there is any cases that cannot be covered by unit or showcase tests. I cannot think a good exception for golden integration tests but I also don't want to say we don't need it at all without auditing. So I prefer to keep it as should but maybe we can add more details to clarify.

DEVELOPMENT.md Outdated
Comment on lines 46 to 47
2. Golden unit tests
3. Showcase integration tests
Copy link
Member

Choose a reason for hiding this comment

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

Could be helpful to have a quick blurb specify that:
Golden unit tests -> Generate an output file for the specific composer
Showcase integration -> Test the generated library's behavior against a mock server (and then link to the showcase module's README/ DEVELOPMENT.md)

Especially helpful for a first-time contributor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, will do! I was planning to add more detailed explanation but wanted to get some initial feedback first, glad to see someone else also found it useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, PTAL.

@blakeli0 blakeli0 requested review from emmileaf and lqiu96 June 8, 2023 21:55
Co-authored-by: Emily Wang <emmwang@google.com>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 8, 2023

[gapic-generator-java-root] 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
No Duplication information No Duplication information

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 8, 2023

[java_showcase_integration_tests] 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
No Duplication information No Duplication information

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 8, 2023

[java_showcase_unit_tests] 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
No Duplication information No Duplication information

@blakeli0 blakeli0 merged commit dfa9d2b into main Jun 9, 2023
@blakeli0 blakeli0 deleted the add-test-guide branch June 9, 2023 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: s Pull request size is small.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants