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

Support tests of classes which use DynamicGraphQLClient #343

Merged
merged 1 commit into from Aug 11, 2022

Conversation

holly-cummins
Copy link
Contributor

As part of my (second, unpushed) fix for quarkusio/quarkus-github-bot#257, I tried to add a test for a PushToProjects, a class which has the following method signature:

    void pullRequestLabeled(@PullRequest.Labeled GHEventPayload.PullRequest pullRequestPayload,
            @ConfigFile("quarkus-github-bot.yml") QuarkusGitHubBotConfigFile quarkusBotConfigFile,
            GitHub gitHub, DynamicGraphQLClient gitHubGraphQLClient) throws IOException {

That test fails with the following exception

[ERROR] io.quarkus.bot.it.PushToProjectsTest.triageComment  Time elapsed: 0.495 s  <<< FAILURE!
java.lang.AssertionError: The event handler threw an exception: The GraphQL client has not been initialized and should not be accessed.
        at io.quarkiverse.githubapp.testing.internal.EventSenderOptionsImpl.event(EventSenderOptionsImpl.java:111)
        at io.quarkiverse.githubapp.testing.internal.EventSenderOptionsImpl.event(EventSenderOptionsImpl.java:85)
        at io.quarkiverse.githubapp.testing.internal.EventSenderOptionsImpl.event(EventSenderOptionsImpl.java:20)
        at io.quarkus.bot.it.PushToProjectsTest.triageComment(PushToProjectsTest.java:73)
...
Caused by: java.lang.IllegalStateException: The GraphQL client has not been initialized and should not be accessed.
        at io.quarkiverse.githubapp.runtime.MultiplexedEvent.getGitHubGraphQLClient(MultiplexedEvent.java:41)
        at io.quarkus.bot.PushToProjects_Multiplexer.pullRequestLabeled_35c5add42697742b520a6db1fd7be53b4ad45669(Unknown Source)
        at io.quarkus.bot.PushToProjects_Multiplexer_Observer_pullRequestLabeled_35c5add42697742b520a6db1fd7be53b4ad45669_b65c7b03e7d0ca92e5b8d9ccb80a1a828633ab4f.notify(Unknown Source)
        at io.quarkus.arc.impl.EventImpl$Notifier.notifyObservers(EventImpl.java:323)
        at io.quarkus.arc.impl.EventImpl$Notifier.notify(EventImpl.java:305)
        at io.quarkus.arc.impl.EventImpl$1.get(EventImpl.java:103)
        at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1768)
        at io.quarkus.vertx.core.runtime.VertxCoreRecorder$14.runWith(VertxCoreRecorder.java:555)

I couldn't figure out a good workaround at the test level, but updating the mocking support to also mock getInstallationGraphQLClient on GitHubService allows the test to proceed.

I've done a simple mock, on the basis that once the hook is there dependencies can add behaviour if they need it.

@gsmet
Copy link
Member

gsmet commented Aug 11, 2022

Yeah I somehow gave up on this as I was pretty sure I wouldn’t be patient enough to write the GraphQL mocks :)

Let’s see what @yrodiere who wrote this infrastructure think of it!

@holly-cummins
Copy link
Contributor Author

I have to admit that I'm not planning to write any GraphQL mocks either, just some light testing of other aspects of a class that references GraphQL. :)
(Famous last words - first you do some light testing of a class that knows about GraphQL, and the next thing you know you've entirely duplicated GraphQL in the medium of mocks.)

@yrodiere
Copy link
Collaborator

I've done a simple mock, on the basis that once the hook is there dependencies can add behaviour if they need it.

Makes sense, in this case DynamicGraphQLClient is an interface (hurray!) and does not have any getter that may return another mock, so we don't need all the hacks we have for the GitHub client.

No idea how easy it will be to actually mock the client, but I agree one should be allowed to try, at least :)

Let’s see what @yrodiere who wrote this infrastructure think of it!

Looks good to me.

We might want to make it easier by allowing to point to a resource file representing the mocked response, but that can be done as a second step... if someone wants it badly enough to do it :)

Merging, thanks!

@gsmet
Copy link
Member

gsmet commented Aug 18, 2022

@allcontributors add @holly-cummins for code

@allcontributors
Copy link
Contributor

@gsmet

I've put up a pull request to add @holly-cummins! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants