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.

Sign up for GitHub

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

Jump to bottom

fix(java): rename test to be picked up by native profile #723

Merged
merged 20 commits into from
Apr 27, 2022
Merged

fix(java): rename test to be picked up by native profile #723

merged 20 commits into from
Apr 27, 2022

Conversation

mpeddada1
Copy link
Contributor

@mpeddada1 mpeddada1 commented Apr 21, 2022 •
edited by suztomo
Loading

A couple of changes to note:

  1. The maven-surefire-plugin defined in datastore-v1-proto-client/pom.xml was overwriting the one defined in the native profile brought in from java-shared-config, therefore, resulting in the following error: Test configuration file wasn't found. Make sure that test execution wasn't skipped.. Tested this change locally to verify that integration tests are excluded when unit tests are run. Also, java-shared-config brings in the same config defined in the POM: https://github.com/googleapis/java-shared-config/blob/b5e05c6726c60a7ad7c0d9588b4fab69a0b4e6b7/pom.xml#L87

  2. Replaced usages of @Rule public ExpectedException thrown = ExpectedException.none(); with Truth. (background: go/cloud-java-native-image#truth-vs-hamcrest-use-truth)

  3. Renamed test that uses EasyMock ITDatastoreTest -> DatastoreTest as mocking is incompatible with native image compilation atm.

@mpeddada1 mpeddada1 requested review from a team as code owners April 21, 2022 16:39
@product-auto-label product-auto-label bot added size: xs Pull request size is extra small. api: datastore Issues related to the googleapis/java-datastore API. labels Apr 21, 2022
@mpeddada1 mpeddada1 requested a review from a team as a code owner April 22, 2022 06:01
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: xs Pull request size is extra small. labels Apr 22, 2022
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels Apr 22, 2022
@mpeddada1
Copy link
Contributor Author

The Kokoro Native Image job is now passing

@mpeddada1
Copy link
Contributor Author

However, the unit tests seem to be failing with the following error:

Trying to download...
Downloading Java 17.0.3+7 (Zulu) from https://cdn.azul.com/zulu/bin/zulu17.34.19_1-ca-jdk17.0.3-linux_x64.tar.gz ...
Error: Unexpected HTTP response: 404

@mpeddada1 mpeddada1 requested a review from suztomo April 22, 2022 21:51
@mpeddada1
Copy link
Contributor Author

All tests are now passing.

suztomo
suztomo reviewed Apr 25, 2022
View reviewed changes
datastore-v1-proto-client/src/main/resources/META-INF/native-image/native-image.properties
@@ -0,0 +1 @@
Args = --enable-url-protocols=https,http
Copy link
Member

Choose a reason for hiding this comment

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

Does this library use HTTP (not HTTPS) somewhere in users' environment? (What's the error without this argument?)

If the request is only for testing, then this file should be under test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the stacktrace, this is coming from com.google.datastore.v1.client.RemoteRpc:

 Caused by: java.lang.IllegalArgumentException: java.net.MalformedURLException: Accessing an URL protocol that was not enabled. The URL protocol https is supported but not enabled by default. It must be enabled by adding the --enable-url-protocols=https option to the native-image command.
       com.google.api.client.http.GenericUrl.parseURL(GenericUrl.java:679)
       com.google.api.client.http.GenericUrl.<init>(GenericUrl.java:125)
       com.google.api.client.http.GenericUrl.<init>(GenericUrl.java:108)
       com.google.datastore.v1.client.RemoteRpc.resolveURL(RemoteRpc.java:161)
       com.google.datastore.v1.client.RemoteRpc.<init>(RemoteRpc.java:62)

Normally this configuration would come from gax, but this module doesn't import it so we had to add this setting in.

Copy link
Member

@suztomo suztomo Apr 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I believe RemoteRpc constructor takes an HTTP URL. Where is the value defined? (My guess is it's only for tests)

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 error is coming from RemoteRpc.resolveURL but it's used in RemoteRpc#call which is later referenced in Datastore. For this reason, I think we would still need it in the main configuration?

@suztomo suztomo requested a review from kolea2 April 25, 2022 13:40
@suztomo
Copy link
Member

suztomo commented Apr 25, 2022

@kolea2 This is a followup from our conversation after the Wednesday meeting.

suztomo
suztomo approved these changes Apr 25, 2022
View reviewed changes
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.

If the allow HTTP is passed in GAX already, then no need to reject it here.

@mpeddada1
Copy link
Contributor Author

Thank you all so much!

@mpeddada1 mpeddada1 merged commit 3a30e75 into main Apr 27, 2022
@mpeddada1 mpeddada1 deleted the rename-test branch April 27, 2022 18:33
@release-please release-please bot mentioned this pull request Apr 27, 2022
Merged
gcf-merge-on-green bot pushed a commit that referenced this pull request May 23, 2022
@release-please
chore(main): release 2.5.0 (#729)
c04b37e 
🤖 I have created a release *beep* *boop*
---


## [2.5.0](v2.4.0...v2.5.0) (2022-05-23)


### Features

* add build scripts for native image testing in Java 17 ([#1440](#1440)) ([#739](#739)) ([252a174](252a174))
* add ReadOption.ReadTime to support timestamp reads. ([#712](#712)) ([06bb08f](06bb08f))


### Bug Fixes

* **java:** rename test to be picked up by native profile ([#723](#723)) ([3a30e75](3a30e75))


### Documentation

* **samples:** remove unused dependency ([#730](#730)) ([5185691](5185691))


### Dependencies

* update dependency com.google.cloud:google-cloud-shared-dependencies to v2.11.0 ([#737](#737)) ([8eb0c5f](8eb0c5f))
* update shared deps to 2.12.0 ([#740](#740)) ([5c3676e](5c3676e))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kolea2 kolea2 kolea2 approved these changes

@suztomo suztomo suztomo approved these changes

Assignees
No one assigned
Labels
api: datastore Issues related to the googleapis/java-datastore API. size: m Pull request size is medium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mpeddada1 @suztomo @kolea2