quarkiverse / quarkus-github-app

Develop your GitHub Apps in Java with Quarkus.
https://docs.quarkiverse.io/quarkus-github-app/dev/index.html
Apache License 2.0
65 stars 29 forks source link

Mocking GHIssueComment for UnitTests #461

Closed kanimaru closed 1 year ago

kanimaru commented 1 year ago

Hi is there an Example how to mock GHIssueComment everytime I try:

var ghComment = mock(GHIssueComment.class);
when(ghComment.getId()).thenReturn(123L);

I get:

java.lang.NullPointerException: Cannot invoke "java.lang.Long.longValue()" because the return value of "org.mockito.internal.creation.bytebuddy.MockMethodInterceptor$DispatcherDefaultingToRealMethod.interceptSuperCallable(Object, org.mockito.internal.creation.bytebuddy.MockMethodInterceptor, java.lang.reflect.Method, Object[], java.util.concurrent.Callable)" is null
gsmet commented 1 year ago

/cc @yrodiere

yrodiere commented 1 year ago

Personally I gave up on trying to understand why that kind of problems arise. I upgraded to JDK 17+ and it seems to have solved it.

yrodiere commented 1 year ago

Oh wait, that's a different problem. Probably an inconsequent Mockito setting that results in incorrect default answers... maybe the problem is on our side, maybe it's in your app, I'd have to check.

I'd recommend using this as a workaround:

doReturn(123L).when(ghComment).getId();
kanimaru commented 1 year ago

Oh wait, that's a different problem. Probably an inconsequent Mockito setting that results in incorrect default answers... maybe the problem is on our side, maybe it's in your app, I'd have to check.

I'd recommend using this as a workaround:

doReturn(123L).when(ghComment).getId();

Doesn't work also:

org.mockito.exceptions.misusing.WrongTypeOfReturnValue: 
Long cannot be returned by getId()
getId() should return String

When I use a string I get again:

java.lang.NullPointerException: Cannot invoke "java.lang.Long.longValue()" because the return value of "org.mockito.internal.creation.bytebuddy.MockMethodInterceptor$DispatcherDefaultingToRealMethod.interceptSuperCallable(Object, org.mockito.internal.creation.bytebuddy.MockMethodInterceptor, java.lang.reflect.Method, Object[], java.util.concurrent.Callable)" is null

For more information used JDK (release from SDK MAN):

openjdk 17.0.5 2022-10-18 OpenJDK Runtime Environment GraalVM CE 22.3.0 (build 17.0.5+8-jvmci-22.3-b08) OpenJDK 64-Bit Server VM GraalVM CE 22.3.0 (build 17.0.5+8-jvmci-22.3-b08, mixed mode, sharing)

installed via: sdk install java 22.3.r17-grl

Maven version from IntelliJ: Apache Maven 3.8.1 (05c21c65bdfed0f71a2f2ada8b84da59348c4c5d)

Quarkus version: 2.16.5.Final Mockito Core: 4.11.0

yrodiere commented 1 year ago

org.mockito.exceptions.misusing.WrongTypeOfReturnValue: Long cannot be returned by getId() getId() should return String

That's interesting... @gsmet looks like the removal of bridge methods doesn't work as intended, at least not in this case:

https://github.com/quarkiverse/quarkus-github-app/blob/df81e198613e027270b4cf5cd7f0a8e43621adb7/deployment/src/main/java/io/quarkiverse/githubapp/deployment/GitHubAppProcessor.java#L190-L219

getId is annotated with the expected annotations, as far as I can tell, so the bridge methods should have been removed:

https://github.com/hub4j/github-api/blob/61a959bfc4569ebd0b7fe371b0b625199f209c27/src/main/java/org/kohsuke/github/GHObject.java#L138-L141

gsmet commented 1 year ago

@kanimaru which version of Quarkus GitHub App are you using?

If you have a reproducer, that would be very welcome.

kanimaru commented 1 year ago
    <quarkus.platform.version>2.16.5.Final</quarkus.platform.version>
    <quarkus-github-app.version>1.16.0</quarkus-github-app.version>

    <compiler-plugin.version>3.10.1</compiler-plugin.version>
    <maven.compiler.release>17</maven.compiler.release>
    <surefire-plugin.version>3.0.0-M5</surefire-plugin.version>
    <maven-failsafe-plugin.version>3.0.0-M6</maven-failsafe-plugin.version>
    <lombok.version>1.18.24</lombok.version>
    <assertj-core.version>3.23.1</assertj-core.version>
    <jsonassert.version>1.5.1</jsonassert.version>
    <testcontainers.version>1.17.6</testcontainers.version>
    <versions-maven-plugin.version>2.10.0</versions-maven-plugin.version>
    <rest-assured.version>5.3.0</rest-assured.version>
    <hypersistence-utils-hibernate-55.version>3.2.0</hypersistence-utils-hibernate-55.version>

Meanwhile I stopped writting unit test and started with integrations tests instead.

Is there online a playground where I could write you the reproducer? Should just be a plain new app and then try to call within a Junit5 test.

var ghComment = mock(GHIssueComment.class);
when(ghComment.getId()).thenReturn(123L);

I think there is nothing special with it.

gsmet commented 1 year ago

@kanimaru yes please attach a project as a zip in this issue. You can upload file in comments (see the Attach files... bar just below the comment text field)

BRLunt commented 1 year ago

Hi @gsmet, I tried to make a minimal example and attached it as zip. my-github-app.zip

You can also find it on GitHub with the respective test class here.

Hope that helps.

gsmet commented 1 year ago

Hmmm. There's something fishy, I will have a look.

gsmet commented 1 year ago

OK so the issue is that you are testing things completely outside of our test infrastructure so Quarkus isn't even started. Thus the bridge methods from the GitHub API are not removed from the GitHub API and Mockito gets all confused.

You need to write your tests as Quarkus tests and that will install the proper infrastructure. That explains why we didn't have failures in our own tests.

You should have plenty of examples here: https://github.com/quarkusio/quarkus-github-bot/tree/main/src/test/java/io/quarkus/bot/it .

gsmet commented 1 year ago

Let me know if that answers your question, my guess it that we can close this issue.

BRLunt commented 1 year ago

Hey @gsmet, thanks for your fast reply and explanation. For me, it's clear now.

gsmet commented 1 year ago

Thanks for your feedback.

gsmet commented 1 year ago

BTW, just to be clear, the Mockito issue with GitHub API is not caused by Quarkus. The GitHub API generates bridge methods that have the same parameters but different return types (you cannot write this in your IDE but it's actually valid bytecode) to keep binary backward compatibility with the old versions. It confuses Mockito and that's why in Quarkus GitHub App, we remove the bridge methods from the bytecode so that Mockito can work as expected. It comes with the price that you might have to update your applications from time to time when upgrading the GitHub API but it doesn't come up that often.