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

[V3] feat: Pact should overwrite existing contract files automatically, or at least provide an option to do so #731

Closed
2 tasks done
lviana-menlosecurity opened this issue Aug 16, 2021 · 8 comments

Comments

@lviana-menlosecurity
Copy link

Before making a feature request, I have:

Feature description

Pact should overwrite existing contract files automatically, or at least provide an option to do so.

Use case

I’m seeing some interesting things regarding pact file creation in pact-js v3 beta and I’m wondering if they're expected. I’ll describe the 2 cases I’m seeing:

  1. Let’s say I have 2 consumer tests, and I’ve run them such that the pact file has already been generated locally with both interactions. Now, let’s say that I edit the uponReceiving string slightly in one of those tests and re-run them. It turns out that pact will actually add a third interaction with the updated uponReceiving string to the existing pact file. I was expecting pact to simply regenerate the pact (e.g. overwrite the existing file) with the uponReceiving string updated in the appropriate interaction (meaning, the pact file would still have 2 interactions, rather than 3).
  2. Again, let’s say I have 2 consumer tests, and I’ve run them such that the pact file has already been generated locally with both interactions. Now, let’s say I change a boolean value in the request body of one of them from true to false, and re-run them. It turns out that pact will not regenerate the pact file, which will still have the old true value for the boolean.

The common thread here seems to be that pact files may not be re-generated properly if they already exist. I get it that removing pact files before running tests avoids these issues. However, is there any value in doing, for example, what’s described in issue 2 above, namely not updating the existing pact file with the new body values (that one seems like a bug to me)? I was just wondering why doesn’t Pact overwrite existing pact files automatically, or at least provide an option to do so. For example, in PactV3Options there could be a boolean field, called something like overwrite, which when set to true would automatically overwrite a previously existing pact file with the one generated by the current run.

@TimothyJones
Copy link
Contributor

We do support this. I thought it was well documented, but perhaps it isn't as I can't find it right now.

The option you want is pactFileWriteMode. Have a look at https://github.com/pact-foundation/pact-js#parallel-tests for some discussion.

Part of the problem isn't pact, but rather the way that JS testing frameworks work. When Jest runs, it runs each spec separately and potentially in parallel, without context on whether other specs are being run. When mocha runs, it runs all tests in one execution. We need to be able to support both cases.


Can you elaborate on why you're expecting an update in your first case? Maybe this needs to be better documented, but uponReceiving exists to provide a key for the interaction (for convenience, the key is also intended to be human-readable). So, when you change the uponReceiving string, you're changing the key for the interaction.

I don't think we can implement the behaviour you're asking for in a way that would work for everyone. If we implemented logic that said "if this new interaction is exactly the same as another one, just rename the other one", we might stamp out interactions from other spec files.

Your second case sounds like a bug. Are you able to share some code that reproduces this?

The common thread here seems to be that pact files may not be re-generated properly if they already exist.

They should be. So, if they're not, let's dig deeper.

The advice to remove the spec files first isn't because interactions can't be updated correctly in place, it's because interactions that no longer exist can't always be removed if pactFileWriteMode is update (for example, in Jest tests we can't construct a list of all interactions delivered in this test run without being very restrictive about the way people write their tests).

@lviana-menlosecurity
Copy link
Author

I wasn't aware of pactfileWriteMode so it’s good to know it already exists. And one of the values it can be set to is overwrite, which seems to be exactly what I was looking for. I do see it defined as part of the PactOptions interface:
https://github.com/pact-foundation/pact-js/blob/feat/v3.0.0/src/dsl/options.ts#L50
However, I do not see a similar option for the equivalent PactV3Options interface:
https://github.com/pact-foundation/pact-js/blob/feat/v3.0.0/src/v3/pact.ts#L11
Not only pactfileWriteMode, but many other fields also seem to be missing from PactV3Options. As mentioned in the description, I'm using pact-js v3 beta, so I'd need it defined there in order to be unblocked. So I guess this issue should be about adding the missing fields to PactV3Options.

@TimothyJones
Copy link
Contributor

Ah! Apologies, I'm not sure how I missed that you were asking about v3. It seems clear in your question.

The rust core doesn't have the same lifecycle, so many of the options from the current release are not applicable to the beta release. Having said that, we're trying to get the next release as close as possible to a drop-in release.

One major difference in the lifecycle with the rust core is that the pact file is written at the end of each test (when you call executeTest ). The rust core does support overwrite, but since it would overwrite at each test this is almost certainly not what you want - so we haven't exposed that function in the DSL. I don't think it's going to be practical to do so.

Your first use case is solved by clearing the pact files at the start of the run. I would like to better support the clearing of pact files automatically at the start of the test run, but in js land we have to do that on a per-framework basis. If you could let us know what framework you're using, we can prioritise that one first.

The second use case still sounds like a bug - could you provide an example that we can reproduce?

@lviana-menlosecurity
Copy link
Author

lviana-menlosecurity commented Aug 18, 2021

We use Nx, which by default uses Jest under the covers.
As for an example for the second case, it's trivial and exactly as I said. Let's say we have a simple test like the following:

    it('should get the policy revision', () => {
      const mockValue = {
        state: 'staged'
      };
      provider
        .uponReceiving('a policy revision request to get')
        .withRequest({
          method: 'GET',
          path: '/api/policy/v1/rules',
          query: { timestamp: MatchersV3.fromProviderState('${timestamp}', '1627488400629') }
        })
        .willRespondWith({
          status: 200,
          body: mockValue
        });

      return provider.executeTest(async (mockserver) => {
        const result = await service.v1.rules.getPolicyRevision().toPromise();
        expect(result).toEqual(mockValue);
      });
    });

Then, we run it and the pact file gets generated, with the response body containing a "state" of "staged".
Then, without deleting the pact file, you make a simple change to the value of the "state" field to say "published", and re-run. The test will pass, the timestamp of the pact file will be updated, however the response body will still have the old "state" of "staged", instead of "published". This seems to happen no matter what kind of change you make, meaning that if the pact file already exists it will not be updated with the new values. Now, if I first delete the pact file before running the test the second time, then it works fine.

@TimothyJones
Copy link
Contributor

 MatchersV3.like(mockValue)

^ This is why you're seeing that behaviour. The like matcher matches on type. So, you're saying that your expected response is an object that looks like this:

{
        state: string
}

If you want the pact file to exactly match staged , then change it to:

 body: mockValue

@lviana-menlosecurity
Copy link
Author

Sorry, my bad, even without the like matcher, I still get the same result (meaning, the pact file is not updated with the new values). I'll remove the like from my previous response for clarity.

@mefellows
Copy link
Member

Now, if I first delete the pact file before running the test the second time, then it works fine.

Because of the way the new core works, it can allow several concurrent processes (tests) to be running in parallel on different ports. If there is an interaction conflict (uniqueness is by the combination of a description, verb, path) it doesn't know what to do - i.e. which one should take priority? Neither, that could result in a bug.

I believe it should warn/error in this case, saying that there is a conflict. If not, that should be a bug.

As you've discovered, you should clear all pacts before running tests locally.

Apologies, this does appear to be documented (and is a divergence from the current mainline). There are ways to resolve it, but the trade off is we can't allow concurrency. It's analogous to this behaviour.

@mefellows
Copy link
Member

Closing as not supported. The pact files can be cleared before the tests run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants