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): Use Junit.assert instead of Hamcrest to fix Native Image testing #1746

Closed
wants to merge 14 commits into from
Closed

fix(java): Use Junit.assert instead of Hamcrest to fix Native Image testing #1746

wants to merge 14 commits into from

Conversation

mpeddada1
Copy link
Contributor

@mpeddada1 mpeddada1 commented Mar 11, 2022 •
edited
Loading

This PR is needed to address failures related to Native Image compilation.
Calling mvn test -Dtest=com.google.cloud.spanner.connection.it.ITQueryOptionsTest -Pnative -DfailIfNoTests=false locally results in the following error:

JUnit Vintage:ITQueryOptionsTest:verifiesQueryOptions
    MethodSource [className = 'com.google.cloud.spanner.connection.it.ITQueryOptionsTest', methodName = 'verifiesQueryOptions', methodParameterTypes = '']
    => java.lang.Error: Cannot determine correct type for matchesSafely() method.
       org.hamcrest.internal.ReflectiveTypeFinder.findExpectedType(ReflectiveTypeFinder.java:49)
       org.hamcrest.TypeSafeMatcher.<init>(TypeSafeMatcher.java:40)
       org.hamcrest.TypeSafeMatcher.<init>(TypeSafeMatcher.java:22)
       org.hamcrest.core.SubstringMatcher.<init>(SubstringMatcher.java:13)
       org.hamcrest.core.StringStartsWith.<init>(StringStartsWith.java:13)
       org.hamcrest.core.StringStartsWith.startsWith(StringStartsWith.java:38)
       org.hamcrest.CoreMatchers.startsWith(CoreMatchers.java:516)
       com.google.cloud.spanner.connection.SqlScriptVerifier.verifyExpectedException(SqlScriptVerifier.java:188)
       com.google.cloud.spanner.connection.AbstractSqlScriptVerifier.verifyStatement(AbstractSqlScriptVerifier.java:367)
       com.google.cloud.spanner.connection.AbstractSqlScriptVerifier.lambda$verifyStatementsInFile$0(AbstractSqlScriptVerifier.java:315)
       [...]

GraalVM isn't able to handle more complex assertions in Hamcrest such as assertThat().startsWith() and assertThat().contains(). The Truth framework and Junit, on the other hand, are compatible.

cc @ansh0l @meltsufin

@mpeddada1 mpeddada1 requested a review from a team as a code owner March 11, 2022 00:50
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/java-spanner API. label Mar 11, 2022
@mpeddada1 mpeddada1 requested a review from a team as a code owner March 11, 2022 00:53
ansh0l
ansh0l approved these changes Mar 29, 2022
View reviewed changes
Copy link
Contributor

@ansh0l ansh0l left a comment

Choose a reason for hiding this comment

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

lgtm, Since this is a change of just the assertion library. Adding a do not merge label since we intend to merge all the native image related PRs in a single go.

google-cloud-spanner/src/test/java/com/google/cloud/spanner/connection/SqlScriptVerifier.java Outdated
import static org.hamcrest.CoreMatchers.startsWith;
import static org.hamcrest.MatcherAssert.assertThat;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
Copy link
Contributor

Choose a reason for hiding this comment

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

@mpeddada1 I'm trying to understand the import for Truth: https://github.com/googleapis/java-spanner/blob/aac438e02e841c6ee44362f5f319505d0264ceda/google-cloud-spanner/pom.xml, but can't find it. Can you point me to the right pom?

Copy link
Member

Choose a reason for hiding this comment

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

Is this you're looking for?

java-spanner/google-cloud-spanner/pom.xml

Line 315 in aac438e

<artifactId>truth</artifactId> 

    <dependency>
      <groupId>com.google.truth</groupId>
      <artifactId>truth</artifactId>
      <scope>test</scope>
    </dependency>

@ansh0l ansh0l added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 29, 2022
@mpeddada1 mpeddada1 changed the title fix(java): Use Truth instead of Hamcrest to fix Native Image testing fix(java): Use Junit.assert instead of Hamcrest to fix Native Image testing Apr 4, 2022
@ansh0l
Copy link
Contributor

ansh0l commented May 23, 2022

Closing since native image changes have been released with #1878

@ansh0l ansh0l closed this May 23, 2022
@ansh0l ansh0l reopened this May 23, 2022
@product-auto-label product-auto-label bot added the size: s Pull request size is small. label May 23, 2022
@ansh0l
Copy link
Contributor

ansh0l commented May 23, 2022

Closing since native image changes have been released with #1878

@ansh0l ansh0l closed this May 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@suztomo suztomo suztomo approved these changes

@ansh0l ansh0l ansh0l approved these changes

@meltsufin meltsufin Awaiting requested review from meltsufin

@Neenu1995 Neenu1995 Awaiting requested review from Neenu1995

Assignees
No one assigned
Labels
api: spanner Issues related to the googleapis/java-spanner API. do not merge Indicates a pull request not ready for merge, due to either quality or timing. size: s Pull request size is small.
Projects
None yet
Milestone
No milestone
Development

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