pact-foundation / pact-js

JS version of Pact. Pact is a contract testing framework for HTTP APIs and non-HTTP asynchronous messaging systems.
https://pact.io
Other
1.61k stars 344 forks source link

Matcher of a type or null #51

Closed lippea closed 7 years ago

lippea commented 7 years ago

Is there a way to match the value to a type or null (not empty, not a string)?

mefellows commented 7 years ago

Are you referring to optional types? If so, we don't support this. The article explains why.

lippea commented 7 years ago

@mefellows not optional attributes, but the value could be null, like {"item": null} It mean the "item" must exist, but the value is null, which is not part of the contract

mefellows commented 7 years ago

Hmm I don't believe so. From my perspective, a null value is equivalent to not having it there in the first place.

Out of interest, have you tried?

lippea commented 7 years ago

I tried and it failed on the null values and pass on the other values :) Is this a similar issue? https://github.com/DiUS/pact-jvm/issues/261

mefellows commented 7 years ago

Sorry to say, but it does look like the same one. Leaving open until v3 implementation is completed.

lippea commented 7 years ago

thanks @mefellows , that would be great.

mefellows commented 7 years ago

@bethesque just checking, this is not an available feature correct?

bethesque commented 7 years ago

No. It's because this test doesn't tell you anything. If you always get nulls in test environment, then you think you've passed, but what then the production environment returns a different type to what you expected, and your code blows up. It will not be supported.

atikenny commented 5 years ago

I know this ticket is closed but I ran into this just now and wanted to share a different opinion maybe people find it useful.

While developing frontend applications I ran into a lot of cases where some kind of empty(ish) value was present in the response from the backend that created issues on the consumer side. Usually these were empty arrays, empty objects or a string that says "null". This lead to all sorts of long investigation and some defensive programming as a remedy (which I personally don't really like).

So usually after a couple of meetings with service owners we managed to push for providing nothing instead of an empty value in case some data was actually not available. With a very exaggerated example you can end up with the following.

Response:

{
  "usefulData": { "a": 1, "b": 2, "c": 3 },
  "emptyThing": [],
  "almostUsefulData": {
      "butStillEmpty": [],
      "thisDoesntHelpEither": {
          "exxageratedButHappens": []
      }
  },
  "andTheListGoesOn": [],
  "nowThisIsJustRude": "null",
  "butThisIsNotMuchBetterEitherBecauseIStillDontCare": null
}

Which in the consumer will cause code like:

const almostUsefulData = response && response.almostUsefulData;
const butStillEmpty =
  almostUsefulData &&
  almostUsefulData.butStillEmpty &&
  almostUsefulData.butStillEmpty.length;
const thisDoesntHelpEither =
  almostUsefulData && almostUsefulData.thisDoesntHelpEither;
const exxageratedButHappens =
  thisDoesntHelpEither &&
  thisDoesntHelpEither.exxageratedButHappens &&
  thisDoesntHelpEither.exxageratedButHappens;
const hasData = almostUsefulData && (butStillEmpty || exxageratedButHappens);

if (hasData) {
  // show it
}

I left a bug in the above code just to show how easy it is to make one and hard to figure out where it is :).

Instead of this much simpler one:

const hasData = response && response.almostUsefulData;

if (hasData) {
  // show it
}

BUT: I understand the argument that leaving things out and not testing them can end up causing issues. So what I would do is force a state on the provider when things are available and force another one where they are not and test if things look how I would like them. That way I know that I am ready for the data when I get it and I don't break when I don't have it.

Opinions?

mefellows commented 5 years ago

I like your slightly contrived, a little bit long yet also illuminating example :)

In any case, yes, our standard advice is to make sure you explicitly test the case where values exist, don't exist or are returned as empty values (e.g. null or []). These may or may not be done as part of Pact tests, but we would suggest they are captured in pacts and made explicit back to any provider.

atikenny commented 5 years ago

Thanks for the quick response, sorry for the example but it really is not that far from reality.

So if I understand it correctly the only thing missing from Pact to validate these kind of responses from the provider is something saying:

// GIVEN state: "you don't have data"

// WHEN calling the provider

// THEN "missingData" is not present

So for now all we can do is write pact for when things DO exist and use other forms of testing when things are missing.

mefellows commented 5 years ago

There's nothing stopping you from doing exactly that now. That's the whole point of provider states.

Or am I missing something?

atikenny commented 5 years ago

Probably I am missing something because I couldn't find a way to check with pact for things that should NOT be present. Time to google...

mefellows commented 5 years ago

You would write a test as you stated above with a state indicating so (e.g. given almostUsefulData is empty) and in the willRespondWith you would simply omit the fields you were previously expecting. Now you have a contract with two interactions - one with and one without data. You then must ensure your provider will honour those two different states.

kxramas commented 4 years ago

It is not clear to me if there are 10 fields in json which can either have a value or not have value or takes only defined format then should we write additional 30 pact tests each with a state of a field testing all combinations of possible values for all fields ?

Thanks

mefellows commented 4 years ago

@kxramas It's hard to say without more context - it's unusual that every field is optional however, and usually there are different sub-shapes in the payload you might be separately interested in which could form the basis of different test expectations. For example, a User may have a "profile" sub-object which is optional (perhaps that information is collected later on). Often times, this can be improved by having links to the resource if available (e.g. using HAL links), but sometimes you can't change the implementation.

If they are all indeed optional, I'd first look at how you might be able to improve the API. The key thing is how your client behaves in the different circumstances - will it not work if none of the fields came back, or just a few? That might help you in your endeavour.

bethesque commented 4 years ago

If it was me, I'd do one test which had every field populated, and one that had the minimum that could be expected. Really, what you're testing in this case is that code that deserialises the response into your local domain objects can handle both scenarios.

maximvl commented 3 years ago

Hello, I would like to bring this up again because this is a serious limitation of the tool. In our company we have api responses with hundreds of fields of which dozens are optional and can be null. Unless I'm missing something about your position on optional types you suggest to write hundreds of cases for each combination of null/type fields which is terribly unpractical. Also you must understand that after a certain size projects will not adapt their code and style just for one tool. I understand the desire for ideal world where everything is perfectly typed but in fast growing environment this simply does not happen. As you see the amount of requests for this feature I suggest you to reconsider it. Thank you.

mefellows commented 3 years ago

Hi Max, thanks for the feedback. I can certainly empathise.

At this stage, we're exploring the idea of "provider driven" contracts, where more of a schema comparison from the provider side against consumer test examples. You could think of this as a contained experiment, and learnings may be applied into the Matcher library. At this stage, as you can see from the above discussion (and Pact's official position on it, changing the core Matchers to support this is unlikely to change quickly.

See https://github.com/pactflow/roadmap/issues/4 (Pactflow specific) and https://github.com/pact-foundation/pact-specification/issues/76 for future thinking that encompasses your suggestion.

maximvl commented 3 years ago

@mefellows thank you for fast response :)

I found in specification 3 there is a combine field which can have or value with a list of matchers, this seems like a way to implement optional matching, is that correct?

mefellows commented 3 years ago

You're welcome.

With the combine matcher, I believe what you're suggesting is in theory possible, but I'm not entirely sure I'm afraid.

@uglyog @bethesque care to weigh in on this one?

maximvl commented 3 years ago

I checked into the v3 features, I was able to generate specs but it seems the verifier does not support and style of matchers yet. Also I think contract generator does not have an api for combined fields.

bethesque commented 3 years ago

In our company we have api responses with hundreds of fields of which dozens are optional and can be null

Do you need to test all the combinations? How few examples could you use to cover all the fields?

but it seems the verifier does not support and style of matchers yet.

No, it's not supported in v2. Which language are you verifying in?

It sounds to me like Pact is not a good fit for this particular use case. I would look at using the "Atlassian flow" which allows you to verify a pact against an OAS. You can continue to use Pact on the consumer side, but verify using https://www.npmjs.com/package/swagger-mock-validator on the provider side.

bethesque commented 3 years ago

Or wait until Pactflow's "provider driven" flow is out, as Matt suggested.

mefellows commented 3 years ago

You can continue to use Pact on the consumer side, but verify using https://www.npmjs.com/package/swagger-mock-validator on the provider side.

It's also worth expanding on that briefly, because it might sound like a limitation (still having to use Pact on the consumer side). But actually, it allows you to vastly expand your suite of consumer tests without burdening the provider side, because all it does is compare schemas. It also allows this specific use case.

maximvl commented 3 years ago

@bethesque thanks for response,

How few examples could you use to cover all the fields?

So this is exactly my point, since different fields are optional Pact contracts will not cover the responses our backend can produce. Unless I'm missing something.

I understand this is not supported in v2 so I'm looking forward v3, actually I'm preparing a pr because it seems that it can be done.

@mefellows About the swagger mock validator - actually I'm probably not fully understanding things, currently our setup is like this that we are not using broker and just running pact-verifier in our backend CI pipeline, but the idea is to expand this. You are saying "all it does is compare schemas" - is that similar to Pact verifier? Or there are more things happening in the verifier?

mefellows commented 3 years ago

@maximvl apologies for the delay.

The basic flow:

  1. Provider validates their API matches a given OAS (in the future, we'll support other modes here) using whatever tool they choose (e.g. Dredd)
  2. Provider publishes OAS to Pactflow, along with the evidence in one
  3. Provider runs can-i-deploy to ensure the contract is compatible with its consumers

Separate to the above (before/after) the consumer runs its own pipeline:

  1. Consumer writes and publishes contracts (via Pact) as they do today
  2. Consumer runs can-i-deploy, which will check if the given provider's OAS is compatible with the pact

The pact contract is now statically compared to the OAS, using a type of schema verification. If a field is null or not in the pact, so long as the OAS schema allows it it is ✅ .

See also https://github.com/pactflow/roadmap/issues/4. Please note that this is a feature that will be unique to Pactflow. It may be made open via the OSS pact broker eventually, but it's not currently planned.

pavelkomarov commented 2 years ago

What if instead of "oops everything could fall through because it's all null", you implement syntax like

[
  1639587840000,
  77.8544510992,
  0.3886455188,
  "upright",
  "walking" & null
]

where the test case then has to hit both things on either side of the & condition?

mefellows commented 2 years ago

I think we've talked about something like that. You would still need a way of communicating those two states to the provider, so presumably it would need to map to a provider state (e.g. by convention). How do you envisage the consumer DSL and provider side working here?

akanksha-centric commented 2 years ago

Hello, I am also struggling with similar kind of issues, I am writing test case for an api having 100 of filed and there are multiple optional/null value fields that some time return value or some time not. I have also read pact optional attribute but it is not helpful I don't want to create multiple scenarios, What are the other why to validate both attribute having null or some value.

and is there are any way to compare only schema of expected response not value?

mefellows commented 2 years ago

Hi @akanksha-centric.

No, Pact doesn't have a separate way of doing this other than what is discussed here.

If you want a purely schema based test, you might be interested in a feature releasing next week at Pactflow which does exactly this: https://pactflow.io/bi-directional-contract-testing/#register

pavelkomarov commented 2 years ago

Pact should allow nullable fields. I don't care what the orthodoxy says. The answers given here are objectively unhelpful. Nullable fields are such a basic thing apps do. At my company we literally cloned and modified a pact verifier, because we really needed this functionality.

mefellows commented 2 years ago

Pact should allow nullable fields. I don't care what the orthodoxy says. The answers given here are objectively unhelpful. Nullable fields are such a basic thing apps do. At my company we literally cloned and modified a pact verifier, because we really needed this functionality.

That is the beauty of open source! You can disagree and fork.

I know it's hard, but as soon as you start to follow the line of thinking it quickly moves the framework away from what is uniquely powerful with Pact and its core design - and at that point you may as well do schema validation through other means and tools. We've taken the choice that it's best to be strict, even if that means it doesn't work for everyone.

dantswain commented 2 years ago

Hi I just wanted to add a couple data points here. I don't think my use case completely makes the argument one way or another but I haven't seen this perspective represented in the discussion:

mefellows commented 2 years ago

Thanks @dantswain. I don't think this changes the philosophical argument here but another data point is helpful.

The reality is, there are other solutions to test this kind of contract (the most common and readily available being a schema check). After many debates, we always land on "why would Pact support this when there are other tools that can do it" with the additional "now it's a different mode we need to support / document / maintain" and finally, we have to explain the loss in fidelity that that kind of testing enables. That is, a schema check enables a wide range of false positives (see https://pactflow.io/blog/schemas-are-not-contracts/ for a wider post on the matter).

daniel-hyun-chae commented 1 year ago

For me, it makes sense to allow optional field definition in contract because that's the truth about the contract. It should be up to the provider test to verify all different cases against the contract. Maybe some matrix such as contract coverage will be awesome to see how much of the contract is covered by the provider tests. The capability to write negative tests on provider side would help this even further.

I don't understand why Pact is trying to prevent optional field on principle level. On consumer side, sometimes, it doesn't really matter whether something is null or string. If being scenario specific is important, more targeted contract can be written only for those special cases. In general, being able to use type definition or schema to create contract is a great help for the maintenance. So, it would be nice to have the freedom to choose it.

mefellows commented 1 year ago

It should be up to the provider test to verify all different cases against the contract.

No, that is not what contract testing is about. The whole point of capturing the consumer contract is to avoid the ambiguity in such cases.

On consumer side, sometimes, it doesn't really matter whether something is null or string.

It's quite simple, if it doesn't matter, exclude it from the contract altogether.

If being scenario specific is important, more targeted contract can be written only for those special cases.

Exactly.

In general, being able to use type definition or schema to create contract is a great help for the maintenance. So, it would be nice to have the freedom to choose it.

Pact is simply not a schema checking tool - there are plenty of schema checking tools out there, Pact is not one of them.

For a long form on why schemas are not contracts, see this post: https://pactflow.io/blog/schemas-are-not-contracts/.

I'm going to lock this conversation, the position from Pact is clear on this topic.