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

Only one @Pact clause can be used #148

Closed
C-Otto opened this issue Aug 4, 2015 · 17 comments
Closed

Only one @Pact clause can be used #148

C-Otto opened this issue Aug 4, 2015 · 17 comments
Labels
bug Indicates an unexpected problem or unintended behavior

Comments

@C-Otto
Copy link

C-Otto commented Aug 4, 2015

With the new 3.0 release, only the last method defining a pact using the @Pact annotation (for the same producer/consumer pair) is regarded. Swapping the order of the two methods in the file, not changing anything else, causes the other definition to be available, while the definition that worked before now is ignored.

@uglyog
Copy link
Member

uglyog commented Aug 4, 2015

Good point. The previous implementation would work as long as the provider state was unique. I'll key it on consumer+provider name.

@C-Otto
Copy link
Author

C-Otto commented Aug 4, 2015

As said, I have the same producer/consumer pair in both of my tests (and in general I have several tests for the same pair in a single class, and only for this pair).

@uglyog
Copy link
Member

uglyog commented Aug 8, 2015

How are the test methods selecting the pact to use if you have multiple defined? Where you using the provider state to distinguish them?

@C-Otto
Copy link
Author

C-Otto commented Aug 8, 2015

I don't know how the pact is chosen, I guess this is part of the problem. My PactVerification annotation just repeated the producer/consumer definition (which, as I said, is redundant/ugly in my setting). I think the state was deprecated in the new PactVerification annotation? I don't have access to the code right now, so I'm replying based on my (bad) memory.

Could you maybe provide a working example of a test class including two pact+test pairs (for the same producer/consumer)?

@uglyog
Copy link
Member

uglyog commented Aug 9, 2015

Why do you need a test class with more than one pact+test pairs for the same producer/consumer. I'm just trying to understand your use case. The pact DSL allows the interactions to be chained, so you can define them all with one pact method. That is how I have setup all my consumer tests.

@C-Otto
Copy link
Author

C-Otto commented Aug 9, 2015

The use case would be something like testing a GET and testing a POST. Those two requests naturally have their own tests. My intuition is to define the pact fragments in two different methods. Even if it were possible to use one method (chaining the fragments), this would be counter-intuitive. The chaining would impose an order, and as an uninformed reader of the code I'd expect BOTH requests to be served, in a specific order.

In other words: in order to be able to write clean code, I'd like to define the fragments without chaining (as it was possible in previous releases, if I remember correctly).

@uglyog uglyog added the bug Indicates an unexpected problem or unintended behavior label Aug 20, 2015
@scho
Copy link

scho commented Aug 27, 2015

This is an issue for us as well. We would like to test different endpoints with different provider states within the same class (have one integration test class per client).

E.g.:
Given two different states: A (resource with id exists), B (resource does not exists => 404)
and two different endpoints: GET /resource/id, DELETE /resource/id

This results in 4 different pacts:

  1. State A, GET /resource/id
  2. State A, DELETE /resource/id
  3. State B, GET /resource/id
  4. State B, DELETE /resource/id

At the moment there is now way to handle this with the new PactProviderRule. The old PactRule at least allows us to use different states and test either pact 1 and 3 or 2 and 4.

My idea would be to give @Pacts a name and use that name in the @PactVerification.

@uglyog
Copy link
Member

uglyog commented Aug 27, 2015

I been thinking about the bast way of implementing this, and I think we could use the method name as an optional value on @PactVerification.

So you can setup your test with two methods that return pact fragments, and then use the @PactVerification to distinguish which one to use.

@scho
Copy link

scho commented Aug 28, 2015

How about creating the pact fragment within the test method? Wouldn't that make it more readable? Why do we need to separate the verification and the creation of a pact fragment at all?

The rule could have a method that returns a builder, which is already configured for the provider/consumer specified when creating the rule itself.

Someone could even configure e.g. headers already within the rule (setting defaults), so you don't need to repeat the same stuff over and over again when creating your pact fragments (I am not sure if this is possible with the current builder structure. It's just a rough idea).

@uglyog
Copy link
Member

uglyog commented Aug 29, 2015

The main problem is the PactRule hides starting and tearing down the mock
server behind the scenes. The mock server has to be started with the Pact
before the test code is executed.

The sequence is:

  1. Define the expected interactions with a pact fragment
  2. Start the mock server with the pact
  3. Execute the consumer test code.
  4. shut down the mock server
  5. validate the status of the test code execution and the mock server

The Pact rule provides steps 2, 4 and 5 behind the scenes.

If you need a different set of interactions for a particular execution of
test code, we will need some way to tie the two together. The previous rule
implementation used the provider state, but that falls down when you need
to define multiple states or have the states shared between different test
code executions.

The idea of setting defaults is a good one. I see if I can come up with a
solution for that.

On 28 August 2015 at 17:49, Georg Meyer notifications@github.com wrote:

How about creating the pact fragment within the test method? Wouldn't that
make it more readable? Why do we need to separate the verification and the
creation of a pact fragment at all?

The rule could have a method that returns a builder, which is already
configured for the provider/consumer specified when creating the rule
itself.

Someone could even configure e.g. headers already within the rule (setting
defaults), so you don't need to repeat the same stuff over and over again
when creating your pact fragments (I am not sure if this is possible with
the current builder structure. It's just a rough idea).


Reply to this email directly or view it on GitHub
#148 (comment).

uglyog pushed a commit that referenced this issue Aug 29, 2015
@uglyog
Copy link
Member

uglyog commented Aug 29, 2015

In mean time, I have enabled defaulting with the pact rule so that the provider does not have to be repeated.

@scho
Copy link

scho commented Aug 29, 2015

If we created the PactFragment within the test method and then tell the rule to start the mock with it, wouldn't that work?

@uglyog
Copy link
Member

uglyog commented Sep 2, 2015

JUnit applies the rules first. The only way to do it would be to have a method to call to start the service with a pact fragment from the test code, but then there would not be much point to using the @PactProviderRule because that is essentially what it does.

@scho
Copy link

scho commented Sep 2, 2015

How about something like this:

@Test
@Pact
public void someTest(){
  rule.loadFragment(this::someFragment)
}

public PactFragment someFragment(PactDslWithState builder){
  //...
}

The rule still has to cleanup the stuff afterwards and could include default configuration for all tests.

@uglyog
Copy link
Member

uglyog commented Sep 2, 2015

That would require modifying the mock server to be able to change the pact after it has started. My idea was:

@Test
@Pact(fragment = 'someFragment')
public void someTest(){

}

public PactFragment someFragment(PactDslWithState builder){
  //...
}

That way the rule can retrieve the pact before starting the mock server.

@scho
Copy link

scho commented Sep 3, 2015

Sure, that would work. Looks fine to me.

What about the defaults? Have you had an idea how to implement it?

@uglyog
Copy link
Member

uglyog commented Sep 11, 2015

I've implemented the fragment override on the PactVerification annotation, for an example:
https://github.com/DiUS/pact-jvm/blob/master/pact-jvm-consumer-junit%2Fsrc%2Ftest%2Fjava%2Fau%2Fcom%2Fdius%2Fpact%2Fconsumer%2Fpactproviderrule%2FPactProviderWithMultipleFragmentsTest.java

As the defaults are a separate to this, I've created a new issue to track that: #159

@uglyog uglyog closed this as completed Sep 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

No branches or pull requests

3 participants