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

Quarkus 3 - consumer tests cannot directly access pact MockServer in test methods in dev mode #73

Closed
holly-cummins opened this issue Feb 10, 2023 · 14 comments · Fixed by #225
Labels

Comments

@holly-cummins
Copy link
Contributor

holly-cummins commented Feb 10, 2023

With the switch to Quarkus 3, we've moved to running more of the Pact infrastructure in the Quarkus classloader, rather than the system one. One side effect of this is that direct access to the pact MockServer gives classloading conversion errors (on Java 17 and higher):

---- Debugging information ----
message             : No converter available
type                : java.util.concurrent.CopyOnWriteArrayList
converter           : com.thoughtworks.xstream.converters.reflection.SerializableConverter
message[1]          : Unable to make private void java.util.concurrent.CopyOnWriteArrayList.readObject(java.io.ObjectInputStream) throws java.io.IOException,java.lang.ClassNotFoundException accessible: module java.base does not "opens java.util.concurrent" to unnamed module @5eff30f4
converter[1]        : com.thoughtworks.xstream.converters.reflection.ReflectionConverter
message[2]          : Unable to make field private static final long java.util.concurrent.CopyOnWriteArrayList.serialVersionUID accessible: module java.base does not "opens java.util.concurrent" to unnamed module @5eff30f4
-------------------------------

Here is an example of a problematic access:

    public void testPortIsCorrect(MockServer mockServer) {

In general, test code should have no reason to access the mockServer directly, because the consumer tests should be testing the consumer, not the mock. The access here was to actually try and test the pact extension.

Because of that, I believe this is a low priority issue.

holly-cummins added a commit to holly-cummins/quarkus-pact that referenced this issue Feb 10, 2023
Quarkus 3 brings two major changes - the javax/jakarta namespace change, and a change to classloading. Quarkus 2 coredefined many pact modules to be parentfirst. In Quarkus 3, that will be done by this extension. The Quarkus 3 kotlin extension also stopped loading kotlin parent first.

I was able to remove all the parent-first dependencies for the consumer. For the provider, oddly, I had it working with a single parent-first dependency in December, but now I needed the full set. I will continue working to see if I can reduce it down to a minimal set, or having both extensions in the same project will be a problem.

I did have to disable one test, and have raised quarkiverse#73, but I think it's not a serious issue.
@daniel-alonso-sanchez
Copy link

daniel-alonso-sanchez commented Jun 2, 2023

Hi! I'm facing this problem also. Probably because I'm following an outdated tutorial... would anybody be so gentle to tell me how can I do something like this:


@Test
    @PactTestFor(providerName = "content-store-service")
    public void verifyFindDepartmentPact(MockServer mockServer) {
        SampleRestClient client = RestClientBuilder.newBuilder()
                .baseUri(URI.create(mockServer.getUrl()))
                .build(SampleRestClient.class);
        Node node = client.getNode("test", "draft", ID);
        assertNotNull(node);
    }

Without facing the error mentioned in this issue?

Thank you so much in advance!

@daniel-alonso-sanchez
Copy link

Using @MockServerConfig annotation perhaps?

@holly-cummins
Copy link
Contributor Author

Ah, there goes my hope that hardly anyone would hit this. :)

It's a bit inelegant, but you can hand-craft the equivalent of .baseUri(URI.create(mockServer.getUrl())) if you don't use random ports.

For example, if you have

@PactTestFor(providerName = "provider", port = "8096")

then you know your base URL will be something like http://localhost:8096.

It's not mentioned in the Pact docs that I can see, but my IDE is showing port as deprecated. Certainly, hardcoding the port isn't as elegant as using a random one. It does create a risk of interference between test instances and real instances if you choose the same port, or between pact and 'normal' mocks in a test run.

On the other hand, one advantage of hardcoding the port is it means you could do something like this instead of using the rest client builder

@ExtendWith(PactConsumerTestExt.class)
@PactTestFor(providerName = "content-store-service", port = "8096")
@QuarkusTest // Needed to enable dependency injection of the rest client
public class MyTest {

    @RestClient
    @Inject
    SampleNorsuRestClient client;
    
    @Test
    public void verifyFindDepartmentPact() {
        Node node = client.getNode("test", "draft", ID);
        assertNotNull(node);
    }

You would also need something like this in your application.properties

quarkus.rest-client.norsu-api.url=http://localhost:8096/  

and this on the actual SampleNorsuRestClient

@RegisterRestClient(configKey = "norsu-api")

A final note is to be careful of spending too much energy testing your RestClient; it will just be a pass-through to the mock you defined using Pact. So it's good to check what you think you defined the mock to do is actually what the mock is doing, and it's good to check the types broadly match up with the endpoints on the other end ... but it's testing a mock.

@daniel-alonso-sanchez
Copy link

Wow! Thank you so much for your suggestion, @holly-cummins !! I'll give a try in a shortwhile!!

BTW, I've changed my original message a little bit, just to remove the "Norsu" name (it's some kind of a keyword in my company).
I know it's asking too much, but could you also modify your answer to not include that name, if you please?

Thank you very much again and sorry for the inconvenience :(

@markusdlugi
Copy link

Hi @holly-cummins,

we also ran into this issue during our Quarkus 3 migration. I disagree regarding your assessment on the priority - I think injecting the MockServer can be a valid requirement sometimes. For example, in our project we are using a custom Wiremock server that is started as part of Dev Services and acts as a proxy to the Pact Mock Server. This accomplishes two things for us:

  1. We can point our application's REST Client to Wiremock, where we have more control due to Dev Services integration, so we also get rid of the programmatic RestClientBuilder.
  2. Additionally, Wiremock is a little more verbose regarding its requests and responses than the Pact MockServer, so it's easier to spot mismatched requests (maybe an idea for Diagnosing consumer failures is really hard #60?).

That aside, the workaround of setting a hard-coded port and avoiding to inject the MockServer does work, but it's still problematic, especially when using Pact in multiple services that run on the same CI.

Is there anything we can do here to get this fixed? I saw that there were similar issues with Quarkus Testing in the past that have been worked around (see for example quarkusio/quarkus#24872), maybe something similar could be done here? Or do we have to wait for JUnit 5.10 to drop so we can fix the classloader stuff (as mentioned in quarkusio/quarkus#22611)?

@holly-cummins
Copy link
Contributor Author

Thanks for the great analysis, @markusdlugi. I love that idea about the Wiremock proxy. I agree, adding an extra proxy does sound like a good way forward for #60. Do you have any sample code you can share? I can't totally picture what you mean about the RestClientBuilder (but I agree it sounds good).

I also agree that I guessed wrong about the priority! My assumption has been that it needs JUnit 5.10 for a fix, and I'm planning to prepare a PR for that, so that it's ready to drop as soon as 5.10 GAs. However, it's been a while since I've looked at the issue, so I'll try again. I did try extending the approach from quarkusio/quarkus#22611 but I stopped after adding support about ten more classes, because it seemed like I was never going to get to the end of the list.

@holly-cummins
Copy link
Contributor Author

On the bright side, whether we fix it or not, we get to call the issue "The Clone Wars."

@Ladicek
Copy link

Ladicek commented Jul 11, 2023

I'm no expert here and probably don't know what I'm talking about, but if obtaining the URL from the MockServer is the most common thing one needs, perhaps this Quarkus extension (or rather the JUnit extension this Quarkus extension provides) could allow "injecting" the URL into the test directly?

@holly-cummins
Copy link
Contributor Author

I think that's definitely where we want to go, @Ladicek. At the moment, any Quarkus-driven injection doesn't work because of classloader problems (the tests run from the un-instrumented test class, not the one updated with Quarkus magic). Once that's sorted out (which I hope will be possible once we move to JUnit 5.10), all sort of developer experience improvements should be possible.

@holly-cummins
Copy link
Contributor Author

Getting closer ... with my prototype on quarkusio/quarkus#34681 the MockServer parameters are now working.

@markusdlugi
Copy link

Sounds good, thanks a lot for all the experimentation :)

@holly-cummins
Copy link
Contributor Author

It looks like this may be fixed by quarkusio/quarkus#40601 (although some other scenarios are broken by that same change!). That change will hopefully be available in Quarkus 3.11, if a needed JUnit upgrade releases in time.

@holly-cummins
Copy link
Contributor Author

I believe this is fixed in Quarkus 3.12, but leaving this open to track making a new branch with that level of Quarkus as the minimum version and enabling tests.

@holly-cummins
Copy link
Contributor Author

#224 moves the minimum version to Quarkus 3.13.0, and with the serialization changes which come with that version of Quarkus, this scenario works properly. I've enabled the tests in #225, and we should be good going forward. Thanks, @dmlloyd!

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

Successfully merging a pull request may close this issue.

4 participants