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: declare test-jar in GAX's testlib artifacts #1474

Merged
merged 8 commits into from
Mar 10, 2023

Conversation

JoeWang1127
Copy link
Collaborator

@JoeWang1127 JoeWang1127 commented Mar 9, 2023

Thank you for opening a Pull Request! For general contributing guidelines, please refer to contributing guide

Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #1144 ☕️

@conventional-commit-lint-gcf
Copy link

conventional-commit-lint-gcf bot commented Mar 9, 2023

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@product-auto-label product-auto-label bot added the size: xs Pull request size is extra small. label Mar 9, 2023
@@ -374,6 +374,7 @@
<dependency>
<groupId>com.google.api</groupId>
<artifactId>gax</artifactId>
<type>test-jar</type>
Copy link
Member

Choose a reason for hiding this comment

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

Can you add source code comment why this test-jar is important?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done.

@suztomo
Copy link
Member

suztomo commented Mar 10, 2023

Assuming this is the correct fix, will we need any change in the users of GAX’s testlib-classifier users?

@JoeWang1127
Copy link
Collaborator Author

Assuming this is the correct fix, will we need any change in the users of GAX’s testlib-classifier users?

I think they need to add <type>test-jar</type> in their pom file.

I found out this in the failing Kokoro checks

[ERROR]   The project com.google.cloud:google-cloud-accessapproval:2.13.0 (/tmpfs/src/github/gapic-generator-java/google-cloud-java/java-accessapproval/google-cloud-accessapproval/pom.xml) has 3 errors
[ERROR]     'dependencies.dependency.version' for com.google.api:gax:jar:testlib is missing. @ line 80, column 21
[ERROR]     'dependencies.dependency.version' for com.google.api:gax-grpc:jar:testlib is missing. @ line 86, column 17
[ERROR]     'dependencies.dependency.version' for com.google.api:gax-httpjson:jar:testlib is missing. @ line 92, column 17

@suztomo
Copy link
Member

suztomo commented Mar 10, 2023

My own confirmation.

Remove local Maven repository cache: $ rm -rf ~/.m2/repository/com/google/api/gax*

Checkout main (not this pull request):

suztomo@suztomo-macbookpro.roam.internal ~/gapic-generator-java
~/gapic-generator-java $ git checkout main
Already on 'main'
Your branch is up to date with 'origin/main'.
suztomo@suztomo-macbookpro.roam.internal ~/gapic-generator-java
~/gapic-generator-java $ git pull
Already up to date.

Open gapic-generator-java project in IntelliJ and tried running the gapic-generator-java module's unit tests. It shows the error.

Screenshot 2023-03-10 at 10 19 09 AM

Now checkout this pull request.

~/gapic-generator-java $ gh pr checkout 1474

Run the unit tests in IntelliJ. It succeeded.

Screenshot 2023-03-10 at 10 25 48 AM

@suztomo suztomo changed the title chore: add test jar chore: declare test-jar in GAX's testlib artifacts Mar 10, 2023
@suztomo
Copy link
Member

suztomo commented Mar 10, 2023

@blakeli0 Can you confirm this pull request fixes the problem in your IntelliJ? (It worked form me)

@@ -48,6 +48,7 @@
<groupId>com.google.api</groupId>
<artifactId>gax</artifactId>
<version>2.23.3-SNAPSHOT</version><!-- {x-version-update:gax:current} -->
<type>test-jar</type>
Copy link
Member

Choose a reason for hiding this comment

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

@JoeWang1127 'dependencies.dependency.version' for com.google.api:gax:jar:testlib is missing. @ line 80, column 21 came from the BOM's declaration of the artifacts. Do you really need that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this line is needed to bring the test code.

Copy link
Member

Choose a reason for hiding this comment

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

A BOM doesn't determine what code goes to JAR file.

Copy link
Collaborator Author

@JoeWang1127 JoeWang1127 Mar 10, 2023

Choose a reason for hiding this comment

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

There's an error in gapic-generator-java module if I delete this line and reload the project:

'dependencies.dependency.version' for com.google.api:gax-grpc:test-jar:testlib is missing.

Copy link
Member

Choose a reason for hiding this comment

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

OK it’s for IntelliJ. Then it makes sense.

@blakeli0
Copy link
Collaborator

@blakeli0 Can you confirm this pull request fixes the problem in your IntelliJ? (It worked form me)

Yes, I verified that it's working now with Joe's fix. Thank you so much @JoeWang1127 !

@product-auto-label product-auto-label bot added size: s Pull request size is small. and removed size: xs Pull request size is extra small. labels Mar 10, 2023
<groupId>com.google.api</groupId>
<artifactId>gax</artifactId>
<version>2.23.3-SNAPSHOT</version><!-- {x-version-update:gax:current} -->
<classifier>testlib</classifier>
Copy link
Member

@suztomo suztomo Mar 10, 2023

Choose a reason for hiding this comment

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

Trying an additional entry that does not carry "test-jar".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@suztomo thanks for the help.

@sonarcloud
Copy link

sonarcloud bot commented Mar 10, 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

@JoeWang1127 JoeWang1127 added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 10, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 10, 2023
@sonarcloud
Copy link

sonarcloud bot commented Mar 10, 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

@JoeWang1127 JoeWang1127 added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 10, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 10, 2023
@JoeWang1127 JoeWang1127 marked this pull request as ready for review March 10, 2023 19:58
@JoeWang1127 JoeWang1127 requested a review from a team as a code owner March 10, 2023 19:58
Copy link
Member

@suztomo suztomo left a comment

Choose a reason for hiding this comment

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

Thank you.

@JoeWang1127 JoeWang1127 merged commit 68f24f2 into main Mar 10, 2023
@JoeWang1127 JoeWang1127 deleted the chore/enable-test-jar branch March 10, 2023 23:05
suztomo pushed a commit that referenced this pull request Mar 21, 2023
Source-Link: googleapis/synthtool@7a220e2
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:6d4e3a15c62cfdcb823d60e16da7521e7c6fc00eba07c8ff12e4de9924a57d28
suztomo pushed a commit that referenced this pull request Mar 21, 2023
Source-Link: googleapis/synthtool@7a220e2
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:6d4e3a15c62cfdcb823d60e16da7521e7c6fc00eba07c8ff12e4de9924a57d28
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.

After repository merge IntelliJ is showing some classes missing
3 participants