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

Incorrect "validated" state on Broker when provider test fails with runtime error #1058

Closed
csbiggar opened this issue Mar 25, 2020 · 11 comments
Labels
bug Indicates an unexpected problem or unintended behavior

Comments

@csbiggar
Copy link

When the provider verification test fails due to a runtime error - most commonly we have MissingStateChangeMethod due to getting the state match up wrong - the results still get published to the broker and incorrectly show as verified.

This is pretty misleading, because actually the verification test didn't run so we don't know if it would have passed or not - we've had cases where it wouldn't.

Could you help me problem solve this? Is it a bug or is there a setting I can tweak so that this shows as "not run" or something else on the broker? Our main requirement is that it doesn't show as "passed"

Here's an example using the public broker as suggested - nb the consumer and provider tests are both in the same project for convenience here!

I've added steps to recreate to the readme, let me know if anything is unclear.

https://github.com/csbiggar/pact-example/blob/master/README.md

Many thanks for any help

@csbiggar
Copy link
Author

@csbiggar
Copy link
Author

Just wondered if you had any thoughts on this at all? It's continuing to cause confusion for us, would love some advice about how we might work around it to force the broker to show "not run"

@tdshipley
Copy link

(FYI: I work in the same team as @csbiggar)

I have been caught out a couple of times while working with API consumers by this. It is also made a bit worse for me as consumers typically check the broker for validation status rather than our pipeline. This leads to false positives without the opportunity for us to correct them on the result unless we are extra diligent (which I try to be!).

@joaofslopes
Copy link

👍

@uglyog
Copy link
Member

uglyog commented Apr 4, 2020

Thanks for the example project, it made it easy to diagnose the issue.

The problem is you're using an old version of the pact-jvm-provider-junit5 library (au.com.dius:pact-jvm-provider-junit5_2.12:3.6.11. This has been fixed in a later version.

Changing the dependency in the example project to

    testImplementation "au.com.dius:pact-jvm-provider-junit5:4.0.8"

fixes the issue.

@csbiggar
Copy link
Author

csbiggar commented Apr 4, 2020

Hi @uglyog thanks very much for looking

I've upgraded as you suggested (apologies, should have checked versions first thing!) and I'm still able to recreate the problem , with a subtle difference ...

(1) If the provider verification would pass if the @State was present, and the only issue is the missing @State, then the verification still fails with MissingStateChangeMethod but the result is still published to the broker as success. I've updated the example project to reflect this, so the readme steps to recreate will still recreate this.

(2) If however the provider verification would fail (should the @State be present) and also, the @State is missing - then it looks like it's running the verification first and not worrying about the state check; the verification fails, and shows as failed on the broker. You can recreate this with the example project by changing line 23 in the ProviderVerificationTest to something else , for example

context.target = HttpsTestTarget("www.google.com/abc", 443, "/")

This is better than previously, but, I think it's inconsistent in its judgement on whether missing state is a problem.

If the idea is to force the provider to acknowledge the consumer's "givens" / state requirements, shouldn't a missing state error show up as failed on the broker , even if the test passes without it?

Alternatively, make state truly optional and don't fail the provider verification if it is missing? (log a warning or something instead?)

Hope this makes sense, thanks again, Carolyn

@uglyog
Copy link
Member

uglyog commented Apr 4, 2020

With my testing of your example project, it posted a failed result when the missing state change exception occurred. You can see the result of that here: https://test.pact.dius.com.au/matrix/provider/ron.google/consumer/ron.pretend-consumer

It runs all the state change methods first. If any of those fail, it should abort at that point with an error.

Can you enable debug level logs and post them here?

@csbiggar
Copy link
Author

csbiggar commented Apr 4, 2020

Yep let me see if I can figure out how to do that ;)

But also ... your verification is from some hours ago, perhaps you could pull the latest of the example code and try again? Until I tweaked about an hour ago, I think the verification was giving a genuine failure - ie (2) in my comment above - when I upgraded it just now i also changed it so it should pass if state is present - ie (1) in my comment above.

Will update when I've figured out debugging ...

@csbiggar
Copy link
Author

csbiggar commented Apr 6, 2020

Here's the logs:

1-pact-verification-should-pass-but-missing-state.txt

  • scenario 1 above - you can recreate this with the example project (pull the latest). State is missing, but broker shows green

2-pact-verification-genuine-failure.txt

  • scenario 2 above - make a small change to the target url in the example project to see this, eg
    context.target = HttpsTestTarget("www.google.com/abc", 443, "/"). State is missing but also there is a genuine verification error, broker show red

@uglyog
Copy link
Member

uglyog commented Apr 10, 2020

I think I have replicated your issue. It looks like you are correct, the missing state change method results in a failure being published, but then the test is run and the results are overwritten with a success.

Thanks for the example project, it made it easy to diagnose.

BTW, the debug logging needed to be enabled for the tests. I did this by adding

testImplementation "ch.qos.logback:logback-core:1.2.3",
            "ch.qos.logback:logback-classic:1.2.3"

to your project dependencies. Then I could see the HTTP requests that were made in the test output.

@uglyog uglyog added the bug Indicates an unexpected problem or unintended behavior label Apr 10, 2020
uglyog pushed a commit that referenced this issue Apr 10, 2020
@csbiggar
Copy link
Author

Thanks so much for fixing this so fast! Have just upgraded our provider tests to use 4.0.10 and it's working perfectly. Much appreciated!

uglyog pushed a commit that referenced this issue Apr 29, 2020
… change method failed #1058

(cherry picked from commit aead5fa)
@uglyog uglyog closed this as completed May 7, 2020
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

4 participants