[DR-3282] Add Pact provider support for integration with WDS consumer #1528
Conversation
First attempt at copying yaml over
Try a better copy of verify pacts yaml
Create reusable Pact.TAG to tag Pact tests, following existing repository convention.
okotsopoulos
changed the base branch from
aj-1233-provider-contract
to
develop
This follows existing convention in SnapshotsApiControllerTest: by only loading in the required SnapshotsApiController into the context and mocking all of its inputs, I want to see what's actually required when setting up the application context before introducing additional env variables. While here, also broke out the reused UUID into a variable.
And enabled standard logging for easier debugging (default is JSON).
Pact tests were failing with: au.com.dius.pact.provider.junitsupport.loader.NoPactsFoundException: No Pact files were found to verify Provider: tdr Source: Pact Broker http://https://pact-broker.dsp-eng-tools.broadinstitute.org
Otherwise, when we've configured our mocks to throw, our controller will not be able to properly handle the exceptions. We've run into this for other MVC unit tests before: see SnapshotsApiControllerTest.java.
This will allow for our deployments to be recorded. This also lays the groundwork to use the same name for both TDR as a consumer and provider, which allows for clearer construction of Pact Broker's network graph.
We needed to make a Terraform change so that the Pact broker credentials could be managed by Atlantis and exposed as GHA env variables.
Was seeing 'Cannot invoke getPreferredVersion because versionConstraint is null'
src/test/java/bio/terra/pact/provider/DataRepoProviderTest.java
Outdated
Show resolved
Hide resolved
This reduces ongoing maintenance burden and better reflects the expectations that WDS has of TDR. Upgraded to latest compatible pact plugins. Moved away from deprecated host and scheme system properties in favor of setting pactbroker.url. Since this is getting set in a test profile, we should no longer need to pass it as an environmentn variable in GHA verify_consumer_pacts.
This allowed us to remove a hardcoded dummy snapshot UUID which needed to match the one that WDS was using. WDS contract tests will need to change to supply this ID.
These names now match the states referenced by our sole consumer, WDS
Only a very small amount of pre-configuration is required to verify Pact contracts: I want a dev to be able to verify contracts without getting bogged down in extraneous set-up.
This makes it so that Pact provider verification will pick up on the expected consumer Pacts from the broker. Included documentation on how to expand the selector if you wish to pick up Pacts from a consumer's feature branch. In coordination with WDS consumer, expanded 'user has access to snapshot with given id' provider state to include a table: this consumer expects that a snapshot retrieved with tables will have at least one table with a name.
We want to set boundaries for how we define our Provider states: if future Consumers end up calling other non-snapshot endpoints, we should set up new corresponding Provider state classes. Note that this name matches existing MockMvc unit test SnapshotsApiControllerTest: I'll seek PR feedback on whether the matching name helps or hurts us.
|
Kudos, SonarCloud Quality Gate passed!
|
okotsopoulos
requested review from
snf2ye,
nmalfroy and
samanehsan
as code owners
| @@ -357,6 +357,10 @@ export AZURE_SYNAPSE_SQLADMINUSER=$(cat /tmp/jade-dev-synapse-admin-user.key) | |||
| export AZURE_SYNAPSE_SQLADMINPASSWORD=$(cat /tmp/jade-dev-synapse-admin-password.key) | |||
| export AZURE_SYNAPSE_ENCRIPTIONKEY=$(cat /tmp/jade-dev-synapse-encryption-key.key) | |||
| export AZURE_SYNAPSE_INITIALIZE=false | |||
|
|
|||
| # Pact contract test settings | |||
There was a problem hiding this comment.
Note for TDR devs --
These are two new environment variables needed to verify Pacts locally.
There was a problem hiding this comment.
Note for TDR devs --
I elected to break out this small bit of configuration rendering into its own file. I know that a pain point for new devs onboarding is not being sure which subset of configurations are actually needed to run unit tests, for example. Interested in your thoughts!
There was a problem hiding this comment.
My inclination would be to keep it all in a single file, rather than dividing things up by tests types. A comment in the script can tell a reader which tests require which environment variables.
| @MockBean private AssetModelValidator assetModelValidator; | ||
|
|
||
| @PactBrokerConsumerVersionSelectors | ||
| public static SelectorBuilder consumerVersionSelectors() { |
There was a problem hiding this comment.
Nice...I'm totally going to copy this.
Do you know what happens when you don't specify this? Is what's defined here also the default?
There was a problem hiding this comment.
All credit to @ichengchang who put up a draft PR to model this selection! I asked that same question in a comment:
My most pressing issue at the time was that since TDR had no Pacts published to it from a consumer's default branch, TDR Pact verification was reporting no Pacts found. I could see Pacts published by WDS against TDR, but they weren't being picked up because they were being published by a WDS feature branch.
There was a problem hiding this comment.
Note for TDR Devs --
You may be thinking, "Don't we already have a SnapshotsApiControllerTest?"
We do! It is a unit test of our SnapshotsApiController. I weirdly liked that these two had the same name (different root directories), it implies a link between the two. But I am absolutely open to renaming this if preferred.
There was a problem hiding this comment.
This seems fine to me, to use the package name to disambiguate tests
Inspired by DataBiosphere/jade-data-repo#1528, this is intended to improve the documentation and first time experience for running pact tests locally.
Inspired by DataBiosphere/jade-data-repo#1528, this is intended to improve the documentation and first time experience for running pact tests locally.
|
Will there be any pact tests in TDR for TPS? |
@samanehsan yes! That work is ticketed separately: https://broadworkbench.atlassian.net/browse/DR-3357 It requires TDR to be set up as a Pact consumer (first time we're doing that) and publish its Pacts against TPS, and for TPS itself to be set up as a Pact provider. TPS is not yet involved in any Pact testing. |
Inspired by DataBiosphere/jade-data-repo#1528, this is intended to improve the documentation and first time experience for running pact tests locally.
| To verify that TDR adheres to the contracts published by its consumers, run: | ||
| ``` | ||
| ./src/test/render-pact-configs.sh | ||
| # Reload your environment variables, e.g. src ~/.zshrc |
There was a problem hiding this comment.
So this assumes that the user has added export PACT_BROKER_USERNAME etc to their .zshrc? I guess overall this doc doesn't really talk about how the rendered configs are exported to the app via environment variables.
|
|
||
| Running Pact tests can be achieved by rendering a small set of Pact-specific configurations first: | ||
| ``` | ||
| ./src/test/render-pact-configs.sh |
There was a problem hiding this comment.
I don't see the value in having this script exist separate from render-configs.sh. Is the idea that most of the time we don't run pact tests locally so it's typically not needed?
| # They are managed by Atlantis and were added to Terraform here: | ||
| # https://github.com/broadinstitute/terraform-ap-deployments/pull/1255 |
There was a problem hiding this comment.
Up to you but I wouldn't leave this in, in case we change how secrets are populated in the future in GitHub. There's nothing specific to the workflow about this as all secrets in GitHub are populated the same way.
| paths-ignore: | ||
| - 'README.md' |
There was a problem hiding this comment.
There's nothing wrong with using paths-ignore but we don't do it elsewhere in TDR and if you wanted to really do this, there are many other .md files which could be listed here.
| - develop | ||
| paths-ignore: | ||
| - 'README.md' | ||
| push: |
There was a problem hiding this comment.
Why is this run on push? That means it will run after a PR is merged to develop, in addition to running before the PR is merged. Under what conditions would this detect an error that would otherwise be missed?
There was a problem hiding this comment.
Ultimately, the verify_cosumer_pacts.yaml workflow needs to report the bumper tag to Pact Broker as the new pact version. The only way to do this will be to publish the verification results once again using the new tag.
There was a problem hiding this comment.
This seems fine to me, to use the package name to disambiguate tests
There was a problem hiding this comment.
My inclination would be to keep it all in a single file, rather than dividing things up by tests types. A comment in the script can tell a reader which tests require which environment variables.
Inspired by DataBiosphere/jade-data-repo#1528, this is intended to improve the documentation and first time experience for running pact tests locally.
https://broadworkbench.atlassian.net/browse/DR-3282
This PR does two things:
datarepoas a Pact provider, and sets up infrastructure for verifying contracts published against it.SnapshotsApiControllerdependencies when setting up Pact Provider states used in Workspace Data Service's published contracts againstdatarepo. Companion PR: AJ-1188 Add wds consumer tests against datarepo provider terra-workspace-data-service#3781. General Pact set up
PRs to
developbranch will now verify thatdatarepoadheres to the contracts published by its consumers, and will then publish the results to the central Pact broker.I've added instructions for running Pact verification locally, still fetching its Pacts from the central Pact broker but not publishing its results.
It's possible to run this verification on a purely local setup, with a local Pact broker. It was a bit cumbersome and I didn't get much extra value from it versus verifying Pacts from the broker, so I elected to leave it out of our checked-in documentation. However, if folks want it codified, I can add it in: https://broadworkbench.atlassian.net/wiki/spaces/IRT/pages/2829680649/Contract+Test+Local+Development
2. Provider states used in WDS contracts
This work was driven by the need to test the protected data journey across service boundaries.
WDS is only calling our snapshot retrieval API as part of this journey, so I've spun up a new
SnapshotsApiControllerTestPact provider. It pairs with WDS' published Pacts: https://pact-broker.dsp-eng-tools.broadinstitute.org/pacts/provider/datarepo/consumer/wds/version/0def2c7d6f55390ce00fc8b02e41fe7844ebfa1fThis is presently set up as a pure unit test of
SnapshotsApiController, so that it can be run quickly with very few prerequisites. The controller's dependencies are all mocked / stubbed.Suggestions for the Pact-curious
Follow-on work
We've discussed concerns that mocking business logic in our Provider states adds maintenance burden, and increases the possibility that mocks could drift from actual app behavior. We want to experiment with sharing mocks between our non-Pact unit tests and Pact Provider states.
Alternatively, some other teams here treat their Provider states as functional tests rather than unit tests -- mocking out external calls and awkward actors but otherwise leaving business logic untouched. These tests run fast and should not be flaky, but can make it hard to pinpoint where a failure happens.