pact-foundation / pact-net

.NET version of Pact. Enables consumer driven contract testing, providing a mock service and DSL for the consumer project, and interaction playback and verification for the service provider project.
https://pact.io
MIT License
842 stars 231 forks source link

Wrong result validation when publishing pacts? #486

Open dominikjeske opened 8 months ago

dominikjeske commented 8 months ago

I'm using 5.0.0-beta.1 to send data to pact. In logs there is info "Pact verification successful" but in logs

2024-01-26T08:26:23.083017Z ERROR ThreadId(09) pact_verifier::pact_broker: Failed to push branch develop for provider version 24.4
2024-01-26T08:26:23.083092Z ERROR ThreadId(09) pact_verifier: Publishing of verification results failed with an error: IO Error - Request to pact broker path '/pacticipants/xxx/branches/develop/versions/24.4' failed: 401 Unauthorized. URL: 'https://test.pactbroker.xxx.pl/'

So my test are green but no pacts reached the server. For now I would have to manually analyze logs and react for this. I think this should be catch by pact-net.

adamrodger commented 8 months ago

@mefellows this looks like it could be an issue with the FFI

mefellows commented 8 months ago

You need to supply a valid PactFlow token (hence the 401). The role associated with the token probably doesn't have one of the contract:data:<scope> permissions. See https://docs.pactflow.io/docs/permissions/#contract_datamanage. See also https://docs.pactflow.io/docs/authorization-help#getting-a-401-unauthorized-when-publishing-or-verifying-pacts.

TL;DR - it's not a Pact .NET issue.

dominikjeske commented 8 months ago

I know how to fix problem with authentication. This issue was about result validation and not authentication issue. I don't want to have situations when there is error but test has successful status.

mefellows commented 8 months ago

I see. The publishing of the verification is unsuccessful, but the tests passed. The overall result should be a non-zero exit status (which presumably is happening).

How would you expect the API to behave here? I guess a clearer error message could help, but I'm wondering if bubbling this to the client libraries (in this case, Pact .NET) is worth doing. How would you both convey the tests passing, but the process failing?

Are you having this problem locally or on CI? In case it's not obvious, you shouldn't be publishing from your local development environment, and so this should be a CI problem. If it fails in CI, the only acceptable thing to do is fail the overall process (so you don't have a false sense of security).

See also https://docs.pact.io/diagrams/ecosystem for background on the Rust core and FFI.

adamrodger commented 8 months ago

From the library perspective I'd want the FFI to return a non-zero status code so I could throw an exception like we do with a few other error cases. Then the user is made aware that the test run as a whole has failed even if every individual test has passed.

adamrodger commented 8 months ago

The current error handling from the verifier FFI is here, where any non zero response would fail the verifier run:

https://github.com/pact-foundation/pact-net/blob/master/src%2FPactNet%2FVerifier%2FInteropVerifierProvider.cs#L236

If "unable to publish results" had its own error code then I could throw an exception with that message, for example. At the very least it could reuse the code for verification failure and then the user would check the logs to see why.

mefellows commented 8 months ago

@uglyog what do you think?

rholshausen commented 8 months ago

This is not a trivial change. The result is the result of the verification. The verifier does not propagate the status of whether it was able to successfully upload the verification results or not.

mefellows commented 8 months ago

This is not a trivial change. The result is the result of the verification. The verifier does not propagate the status of whether it was able to successfully upload the verification results or not.

Perhaps the publish verification results should be an independent function to the verification rather than mixing the concerns? I can see the challenge with that (you would probably need to store a result type, pass an opaque reference back and then use that reference in a separate call). Perhaps just propagating it in the verifier call itself would be sufficient, at least that would result in a broken build and would be more "obvious".

dominikjeske commented 7 months ago

Do you plan to reopen this issue?

adamrodger commented 7 months ago

I've reopened but this requires an FFI change, not a PactNet change.