Add optional argument to @State annotation to specify the endpoint's path it's verifying

[adifalco]

We have pact verifications in the Provider, where the @State annotation are named with arbitrary names, trying to specify the pre-conditions. From the State's name, we cannot determine the endpoint that it's trying to verify.

It's true that in the test class we specify the controller containing the endpoints, but in our case, controllers sometimes have several endpoints.

We have things like this in our contract test classes:

   @State("There is a registered user")
    public void getUserApiState() throws NotFoundException
    {
    }

The controller is setup with mocked dependencies in a setup method annotated with @Before.

When we see that test method, written by someone else, we have no idea which endpoint it's testing. We can work it around to find the target endpoint but we want to have a quick way to have that information, for instance when we review Pull Requests.

We'd like the @State annotation accepts also optional arguments where to specify the endpoint, just as documentation. It should look something like this:

@State(name = "There is a registered user", endpoint = "GET /api/v1/user/{userId}")

... or:

@State(name = "There is a registered user", httpMethod = "GET" path = "/api/v1/user/{userId}")

Thanks!

[sbartsa]

In our case we have multiple consumers requesting the same endpoint. A single test in the provider side satisfies all those consumer tests. So the consumer needs easily to know whether a test for this endpoint exists in the provider side in order to get the relevant providerState to use it in it's own test. I think this is brilliant metadata for identifying this.

[FernandaAldaraca]

@mefellows @uglyog could you give us some feedback on this proposal? We would like to know your opinion 😊 thank you in advanced!

[mefellows]

Personally, it seems sensible to me, but I'm not a JVM maintainer so would defer to somebody with more expertise. I can see why you might like to have the facility.

[bethesque]

If it's just for documentation, why not just use a comment? If it has no actual function, then it could be very confusing, and there's nothing to stop it getting out of sync with the code/contract.

[mefellows]

You probably ought to make it clearer why the endpoint has any bearing on the state itself. i.e if a registered user needs to exist, why would the path or method change what gets added to your db?

From the State's name, we cannot determine the endpoint that it's trying to verify.

States should be independent of requests.

When we see that test method, written by someone else, we have no idea which endpoint it's testing. We can work it around to find the target endpoint but we want to have a quick way to have that information, for instance when we review Pull Requests

I misread this part the first time around. Why does it matter if you don't know which endpoint they are testing?

I can see why we may want to have this sort of information but unsure as to why the annotation should know about it. It feels like something perhaps the broker could simplify for teams (e.g. easily discovering state, which interactions exist for a state etc.)

The second part is related to consumer behaviour which I can empathise with, but I refer you to step 1 of our "Nirvana" docs (https://docs.pact.io/pact_nirvana/step_1#docsNav).

[adifalco]

The issue with just adding a comment is that someone might forget doing it, but having to specify it as an argument as part of the “framework”, would give the provider contract tests a structure, and people will tend more to follow the rule.

As per this documentation, If I'm taking it correctly, the @State name describes a kind of precondition to invoke one specific endpoint, but we need to know what's such endpoint, and that's the relationship I see between the State and the specific endpoint.

Why we'd like to quickly associate the Provider's verifications with their own endpoints?

I can state two examples:

• Usually we receive Pull Requests for Provider-side contract tests, and we have to review the code through BitBucket. As reviewers, how can we validate that they are doing things right if we don't even know which endpoint is that trying to verify? For instance, once it happened that a dev created a verification for one pact that referred to PUT endpoint, but by mistake, in the test, instead of invoking the mocked service that executed the "update", invoked another one that retrieved and returned an instance. Such other service was actually used in a GET endpoint. Because it was a PUT, and the consumer didn't expect any response but just the 200 status code, the test passed. Still, as reviewers, if we would have noticed that the verification test meant actually to verify a PUT endpoint and not a GET, we wouldn't have approved the Pull Request.

• Let's say we have the UserAccount microservice, and there's a controller A that defines 4 different endpoints. We have a contract test that uses the controller A, but it does only two verifications, and the State names don't give us any clue about which of those 4 endpoints they do the verifications on. What about if we want to know which of those 4 endpoints have already contract tests in place? We can work it around, searching by the State name in Bitbucket to get all consumers defining it, but that's not the point.

I'm suggesting to add this argument in the State annotation to specify the endpoint, as an optional argument, but still we could go further and make the RestPactRunner configurable to make tests fail if such argument is not present.

[anto-ac]

A State is not specific to an interaction though. Multiple interactions can reuse the same State.

[adifalco]

Hi @anto-ac I'm not sure if I got your point. Do you mean that many consumers can define many pacts against the same State? If that's what you mean, still I mean that a State corresponds to one specific endpoint, and because of what I explained in the previous comment, plus many developers are working in contract testing at my workplace, I think it would be very useful to include that information within the "framework" structure.

I'd really like to have the opinion from you all about if we can proceed working on this, or if I have to build my own solution to work this around.

[bethesque]

still I mean that a State corresponds to one specific endpoint

This should not be the case. A state should be independent of the endpoint and the http method that's being used.

For instance, once it happened that a dev created a verification for one pact that referred to PUT endpoint, but by mistake, in the test, instead of invoking the mocked service that executed the "update", invoked another one that retrieved and returned an instance. Such other service was actually used in a GET endpoint. Because it was a PUT, and the consumer didn't expect any response but just the 200 status code, the test passed. Still, as reviewers, if we would have noticed that the verification test meant actually to verify a PUT endpoint and not a GET, we wouldn't have approved the Pull Request.

It sounds like you were in the "garbage in, garbage out" situation, and it could have been remedied by following the advice here: https://docs.pact.io/consumer/#beware-of-garbage-in-garbage-out-with-putpostpatch

I'm afraid that your solution does not have my support. You could write your own annotations that acted like comments however.

[adifalco]

Hi @bethesque , I'm afraid I didn't explain the situation properly, so I'll try again, but if my idea had been understood properly already, just ignore this message.

The example I exposed was not a "garbage in, garbage out" situation. The test I referred to was done in isolation. What I meant was the following situation:

The consumer A generated the following Pact against the provider B:

    @Pact(consumer = A)
    fun createPutUserAttributes(builder: PactDslWithProvider): RequestResponsePact {
        val response = builder
                .given("A registered user")
                .uponReceiving("Put user attribute")
                .path("/v1/user/{userId}")
                .method(HttpMethods.PUT)
                .willRespondWith()
                .status(HttpStatus.SC_OK)
        return response.toPact()

In the provider, it's been created the following verification:

@State("A registered user")
void doSomething() {
      when(userController.getUserAttributes(userId).thenReturn(new UserAttributes())
}

The verification test will pass because it still returning 200. But let's say I'm reviewing that code in a PR without any more context (like the endpoint path), I'm gonna think that's correct, when actually it isn't, as the correct implementation should be the following:

@State("A registered user")
void doSomething() {
      when(userController.saveUserAttribute(userId, "ACTIVE").thenDoNothing()
}

[uglyog]

Adding a comment attribute to the state annotation is ok with me, but it will have to be optional. You could also just have a convention to name the method appropriately, i.e.

@State("A registered user")
void aRegisteredUser__ForPut() {
      when(userController.saveUserAttribute(userId, "ACTIVE").thenDoNothing()
}

But I think there is a deeper problem here. The states were designed to be independent of the interaction, and can be shared between tests. For example, "A registered user exists" and "No registered user exists". Having them so tightly coupled to the tests feels wrong.

[uglyog]

This will now log out any comment associated with the provider state

12:23:56.094 [Test worker] INFO  a.c.d.p.p.junit.RunStateChanges - Invoking state change method 'an active account exists':SETUP (I'm a comment)