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.

Sign up for GitHub

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

Jump to bottom

refactor: Use matcher and generator classes #372

Merged
merged 2 commits into from
Dec 18, 2023
Merged

refactor: Use matcher and generator classes #372

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb404e6
refactor: Use matcher and generator classes
tienvx Dec 18, 2023
c9f1c3d
test: Use assertJsonStringEqualsJsonString() for better comparing 2 j…
tienvx Dec 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion example/generators/consumer/tests/Service/GeneratorsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public function testGetGenerators(): void
->setPath('/generators')
->addHeader('Accept', 'application/json')
->setBody([
'id' => $this->matcher->fromProviderState($this->matcher->integer(), '${id}')
'id' => $this->matcher->fromProviderState($this->matcher->integerV3(), '${id}')
]);

$response = new ProviderResponse();
Expand Down
4 changes: 2 additions & 2 deletions example/generators/pacts/generatorsConsumer-generatorsProvider.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
"request": {
"body": {
"content": {
"id": 13
"id": null
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one feels off. Do you have a bit more of an explanation on why the id has changed if the matcher is working?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 2 methods:

  • integer(13) used Type matching rule, with require an integer as example value. This matching rule doesn't support a generator.
  • integerV3(13) used Integer matching rule. This one doesn't require example value. So it support generator.

In this example, I want to test ProviderState generator.

  • Because Type doesn't support generator, I will got the (new) MatcherNotSupportedException exception.
  • Integer, on the other hand, support generator, so we didn't get this exception.

So that's why I replace integer(13) by integerV3(13)

But there is 1 problem:

If I use integerV3(13), the generator ProviderState is still not working (it's because of PHP, not Rust. See GeneratorAwareMatcher)

That's why I replace integerV3(13) by just integerV3().

That's why 13 is replaced by null

Copy link
Contributor Author

@tienvx tienvx Dec 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may ask: Why on the old code, integer(13) still work with the generator ProviderState?

There are 2 reasons:

  • I didn't do any checking in the code, just assign the pact:generator:type = 'ProviderState' into the final json.
  • ProviderState generator is a bit different from other generators:
    • Other generators work on consumer side, so if there is example value (e.g. 13), generator will not have any effects.
    • ProviderState generator work on provider side. The example value (e.g. 13) will be replaced by the value from the provider state anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can update GeneratorAwareMatcher, add additional check if (null === $this->getValue() || $this->generator instanceof ProviderState) {, but I think it's unnecessary condition.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think the additional if statement will make it so that regression and surprise cannot take place, and the API is easier to think about. Computational cost vs user brain cost; but I'll approve (you've been very patient).

Copy link
Contributor Author

@tienvx tienvx Dec 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think the additional if statement will make it so that regression and surprise cannot take place...

It's a good point actually.

regression

Unfortunately, adding a simple condition if (null === $this->getValue() || $this->generator instanceof ProviderState) { doesn't help

surprise

I think we shouldn't allow these code at the first place:

  • $this->matcher->fromProviderState($this->matcher->integer(123), '${id}')
  • $this->matcher->fromProviderState($this->matcher->integerV3(123), '${id}')

Example value can't be used along with generator. Generator will be silently ignored without any notice (except for ProviderState, example value will be ignored). User will be surprised. So I created this PR to handle that #420

},
"contentType": "application/json",
"encoded": false
Expand All @@ -41,7 +41,7 @@
"combine": "AND",
"matchers": [
{
"match": "type"
"match": "integer"
}
]
}
Expand Down
7 changes: 7 additions & 0 deletions src/PhpPact/Consumer/Matcher/Exception/MatcherNotSupportedException.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php

namespace PhpPact\Consumer\Matcher\Exception;

class MatcherNotSupportedException extends MatcherException
{
}
Loading