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

chore: recursive comparison in hermetic build IT #2542

Merged
merged 12 commits into from
Mar 8, 2024
Merged

Conversation

diegomarquezp
Copy link
Contributor

@diegomarquezp diegomarquezp commented Mar 7, 2024

Fixes #2540

Sample error message when running library_generation/test/integration_tests.py

Generation finished successfully. Will now compare differences between generated and existing libraries
******************************
Checking for differences in 'java-apigee-connect'.
  The expected library is in /usr/local/google/home/diegomarquezp/Desktop/sdk-platform-java/library_generation/test/resources/integration/golden/java-apigee-connect.
  The actual library is in /usr/local/google/home/diegomarquezp/Desktop/sdk-platform-java/library_generation/output/google-cloud-java/java-apigee-connect. 
  Some files (found in both folders) are differing:
   -  README.md
   -  proto-google-cloud-apigee-connect-v1/src/main/java/com/google/cloud/apigeeconnect/v1/ConnectionProto.java

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Mar 7, 2024
@JoeWang1127
Copy link
Collaborator

Hi @diegomarquezp, thanks for fixing integration test.

Could you also try to fix the difference in the generated files by changing committish in configuration file?

@diegomarquezp diegomarquezp marked this pull request as ready for review March 8, 2024 05:05
@diegomarquezp diegomarquezp requested a review from a team as a code owner March 8, 2024 05:05
@diegomarquezp
Copy link
Contributor Author

diegomarquezp commented Mar 8, 2024

Hi @diegomarquezp, thanks for fixing integration test.

Could you also try to fix the difference in the generated files by changing committish in configuration file?

Thanks @JoeWang1127, this definitely went right for google-cloud-java, but I'm seeing some strange behavior in java-bigtable that doesn't seem to only depend on the commitish. I'm tracking the issue in #2549 , I suspect is a special treatment case that should be done in generate_library.sh (googleapis and googleapis-gen protos are different from java-bigtable protos)

@JoeWang1127 @blakeli0 what if we go for fixing java-bigtable as a follow up since the purpose of this PR is to fix the testing logic? I have disabled the bigtable test for now.

Copy link

sonarqubecloud bot commented Mar 8, 2024

Please retry analysis of this Pull-Request directly on SonarCloud


END_NESTED_COMMIT
BEGIN_NESTED_COMMIT
docs: [document-ai] updated comments
Copy link
Collaborator

Choose a reason for hiding this comment

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

I added document-ai to the test configuration because I didn't find a commit in alloydb, alloydb-connectors and apigee-connect.

Since we have comments in alloydb, should we remove this entry?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I'm removing document-ai to speed the test up

@JoeWang1127
Copy link
Collaborator

@JoeWang1127 @blakeli0 what if we go for fixing java-bigtable as a follow up since the purpose of this PR is to fix the testing logic? I have disabled the bigtable test for now.

LGTM.

@diegomarquezp diegomarquezp enabled auto-merge (squash) March 8, 2024 15:56
Copy link

sonarqubecloud bot commented Mar 8, 2024

Quality Gate Passed Quality Gate passed for 'gapic-generator-java-root'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

Copy link

sonarqubecloud bot commented Mar 8, 2024

Quality Gate Passed Quality Gate passed for 'java_showcase_integration_tests'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@diegomarquezp diegomarquezp merged commit 89381c7 into main Mar 8, 2024
30 of 31 checks passed
@diegomarquezp diegomarquezp deleted the fix-hermetic-its branch March 8, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hermetic build integration test does not catch differences as git diff does
3 participants