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

Wrong result validation when publishing pacts? #486

Open
dominikjeske opened this issue Jan 26, 2024 · 11 comments
Open

Wrong result validation when publishing pacts? #486

dominikjeske opened this issue Jan 26, 2024 · 11 comments
Labels
bug Indicates an unexpected problem or unintended behavior upstream Indicates that an issue relates to an upstream problem (such as in pact-reference)

Comments

@dominikjeske
Copy link

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
Copy link
Contributor

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

@mefellows
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

@uglyog what do you think?

@rholshausen
Copy link

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
Copy link
Member

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
Copy link
Author

Do you plan to reopen this issue?

@adamrodger adamrodger reopened this Feb 9, 2024
@adamrodger adamrodger added bug Indicates an unexpected problem or unintended behavior upstream Indicates that an issue relates to an upstream problem (such as in pact-reference) labels Feb 9, 2024
@adamrodger
Copy link
Contributor

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

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 upstream Indicates that an issue relates to an upstream problem (such as in pact-reference)
Projects
None yet
Development

No branches or pull requests

4 participants