Skip to content

Conversation

@alicejli
Copy link
Contributor

@alicejli alicejli commented Dec 1, 2023

Follow up PR to #2114 which breaks generation for java-bigqueryreservation due to errant formatting of the proto comments.

cl/587006892 contains the mock for bigqueryreservation's ReservationServiceClent.

I think it could make sense to add an integration test for this as the original integration tests didn't catch this bug, but I also understand that there are already a lot of integration tests. Thoughts?

@alicejli alicejli requested a review from a team as a code owner December 1, 2023 15:52
@product-auto-label product-auto-label bot added the size: xl Pull request size is extra large. label Dec 1, 2023
.append("</td>\n")
.append(" <td>")
.append("<p>" + method.description + "</p>")
.append(CommentFormatter.formatAsJavaDocComment(method.description, null))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make the whole table a String and format with CommentFormatter? This fix should be good for now, but in the future, someone else may add another line with dynamic content that could result in the same bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The challenge with formatting the whole table with CommentFormatter is that it replaces all of the html elements for the table with the HTML-escaped character sequence for them, so the output doesn't stay formatted as a table. You can see the example output here: cl/572240700

@blakeli0
Copy link
Contributor

blakeli0 commented Dec 1, 2023

I think it could make sense to add an integration test for this as the original integration tests didn't catch this bug, but I also understand that there are already a lot of integration tests. Thoughts?

Can we mock this scenario with showcase? By adding the problematic comment to one of the showcase protos.

@alicejli
Copy link
Contributor Author

alicejli commented Dec 5, 2023

I think it could make sense to add an integration test for this as the original integration tests didn't catch this bug, but I also understand that there are already a lot of integration tests. Thoughts?

Can we mock this scenario with showcase? By adding the problematic comment to one of the showcase protos.

Good idea - PR opened: googleapis/gapic-showcase#1391

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 7, 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

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 7, 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
0.0% 0.0% Duplication

@alicejli alicejli requested a review from blakeli0 December 7, 2023 21:44
@alicejli
Copy link
Contributor Author

alicejli commented Dec 7, 2023

@blakeli0 I added a failing test for JavaWriter per our offline discussion. Let me know any additional thoughts, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: xl Pull request size is extra large.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants