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

chore: Replace atLeastAndMostLike by constrainedArrayLike #327

Merged
merged 5 commits into from
Aug 1, 2023

Conversation

tienvx
Copy link
Contributor

@tienvx tienvx commented Jul 25, 2023

Old description

atLeastAndMostLike is my invention from #310, and I don't think it's useful. Instead, I replace it with arrayLike (which is also my invention).

arrayLike do what atLeastAndMostLike do, which is checking both min and max, but it also:

  • Allow any array as example value (so example value doesn't need to be a value appear multiple times)
  • Allow min = 0 (in this case example value doesn't need to be empty array)
  • Min can be null
  • Max can be null
  • Min and max can be both null (now it become $matcher->like())
  • Min and max are not null (atLeastAndMostLike above)

for example (UPDATED):

$fatherUuid = '67b06dc7-af63-41c7-b011-f9bbbe6f5d45';
$motherUuid = '73778b18-f6de-4193-9b5d-970b9a5aacb5';
$childUuid = '977e35b8-ded1-4380-b206-b64c0bc0a312';
$user = [
    'id' => $matcher->uuid($childUuid),
    'gender' => $matcher->regex('male', 'male|female|other'),
    'firstName' => $macher->like('Bettye'),
    'lastName' => $macher->like('Johnston'),
    'birthday' => $matcher->date('yyyy-MM-dd', '2022-11-21'),
    'relationships' => $matcher->like([
        'spouses' => $matcher->arrayLike([$matcher->uuid('b21536d9-b0e4-4475-b9bc-b6d1d0716ba0')], 0),
        'father' => $matcher->uuid($fatherUuid),
        'mother' => $matcher->uuid($motherUuid),
        'children' => $matcher->arrayLike([$matcher->uuid('fb08f967-830b-4c09-a9ea-716a37b8fd85')], 0),
    ]),
];
$father = [
    'id' => $fatherUuid,
    'gender' => 'male',
    'firstName' => 'Deonte',
    'lastName' => 'Zulauf',
    'birthday' =>  '1970-11-21',
    'relationships' => $matcher->like([
        'spouses' => [$motherId],
        'father' => null,
        'mother' => null,
        'children' => [$childUuid],
    ]),
];
$mother = [
    'id' => $motherUuid,
    'gender' => 'female',
    'firstName' => 'Delpha',
    'lastName' => 'Ryan',
    'birthday' =>  '1986-11-21',
    'relationships' => $matcher->like([
        'spouses' => [$fatherUuid],
        'father' => null,
        'mother' => null,
        'children' => [$childUuid],
    ]),
];
$users = $matcher->arrayLike([
    $user,
    $father,
    $mother
], 1)

Link to specifications:

Code taken from pact-js

@mefellows
Copy link
Member

Hey Tien,

Allow min = 0 (in this case example value doesn't need to be empty array)

This scenario is not allowed, see https://docs.pact.io/faq#why-is-there-no-support-for-specifying-optional-attributes.

I'd suggest modelling based on the names here, for consistency across languages: https://github.com/pact-foundation/pact-js/blob/master/src/v3/matchers.ts

@tienvx
Copy link
Contributor Author

tienvx commented Jul 27, 2023

I updated my example so it make more sense why I use min = 0 there

This scenario is not allowed, see https://docs.pact.io/faq#why-is-there-no-support-for-specifying-optional-attributes.

Yes, I agree with it. But it's a pain to implement it. e.g. I will need to split a single test to multiple tests:

  • Users with/without spouses
  • Users with/without father
  • Users with/without mother
  • Users with/without children

I tried min = 0 and it still work, so I need to check is it a bug or not.

I'd suggest modelling based on the names here, for consistency across languages: https://github.com/pact-foundation/pact-js/blob/master/src/v3/matchers.ts

Let me mark this PR Draft for now, I will continue trying to make pact-php more consistent with pact-js

@tienvx tienvx marked this pull request as draft July 27, 2023 15:00
@tienvx tienvx marked this pull request as ready for review July 28, 2023 17:21
@tienvx tienvx changed the title chore: Replace atLeastAndMostLike by arrayLike chore: Replace atLeastAndMostLike by constrainedArrayLike Jul 28, 2023
@YOU54F
Copy link
Member

YOU54F commented Aug 1, 2023

Looks good to me,

Could we update the matchers docs for this matcher

https://github.com/pact-foundation/pact-php/blob/ffi/README.md?plain=1#L154

example from the pact-js

https://github.com/pact-foundation/pact-js/blob/38a68fb4569a20f54160333498b8b0bc20ec7838/docs/matching.md?plain=1#L218

Note:- probably need to update the all the matchers there but can do that in a sep PR

@YOU54F YOU54F self-requested a review August 1, 2023 11:32
Copy link
Member

@YOU54F YOU54F left a comment

Choose a reason for hiding this comment

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

Just a suggestion on updating README, otherwise great work

@YOU54F YOU54F requested a review from mefellows August 1, 2023 11:55
@YOU54F
Copy link
Member

YOU54F commented Aug 1, 2023

Just requesting your eyes across this @mefellows

@tienvx
Copy link
Contributor Author

tienvx commented Aug 1, 2023

Update README.md with constrainedArrayLike matcher only

@YOU54F
Copy link
Member

YOU54F commented Aug 1, 2023

Thanks @tienvx

I'll get this merged and then get 10.0.0-alpha2 out for you

@YOU54F YOU54F merged commit 8a3581b into pact-foundation:ffi Aug 1, 2023
26 checks passed
@tienvx tienvx deleted the replace-matcher branch August 1, 2023 15:05
@mefellows
Copy link
Member

Thanks, the constrained version of the function looks OK to me

Yes, I agree with it. But it's a pain to implement it. e.g. I will need to split a single test to multiple tests:

Yes, it is, but that's kind of the price you pay for those guarantees :)

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

Successfully merging this pull request may close these issues.

None yet

3 participants