pact-net icon indicating copy to clipboard operation
pact-net copied to clipboard

Wrong result validation when publishing pacts?

Open dominikjeske opened this issue 1 year ago • 11 comments
trafficstars

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.

dominikjeske avatar Jan 26 '24 12:01 dominikjeske

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

adamrodger avatar Jan 26 '24 16:01 adamrodger

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.

mefellows avatar Jan 29 '24 00:01 mefellows

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.

dominikjeske avatar Jan 29 '24 09:01 dominikjeske

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.

mefellows avatar Jan 29 '24 10:01 mefellows

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 avatar Jan 29 '24 16:01 adamrodger

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.

adamrodger avatar Jan 29 '24 17:01 adamrodger

@uglyog what do you think?

mefellows avatar Jan 29 '24 22:01 mefellows

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.

rholshausen avatar Jan 29 '24 23:01 rholshausen

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".

mefellows avatar Jan 30 '24 03:01 mefellows

Do you plan to reopen this issue?

dominikjeske avatar Feb 08 '24 10:02 dominikjeske

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

adamrodger avatar Feb 09 '24 16:02 adamrodger