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

New expectations: toHaveKeys() & toContainAll() #142

Closed
wants to merge 5 commits into from

Conversation

alexmartinfr
Copy link
Collaborator

Q A
Bug fix? no
New feature? yes
Fixed tickets -

Adds new expectations:

  • toHaveKeys(array $keys)
  • toContainAll(array $needles)

Inspired by Freek in a recent livestream: https://youtu.be/KXUVnu-fTIM?t=3625

toHaveKeys(array $keys):

Checks for multiple keys at once in the value array.
Will only pass if all of the provided keys are present.
toContainAll(array $needles):

Checks for multiple needles at once in the value.
Will only pass if all of the provided needles are present.
Fixed PHPDocs
Copy link
Member

@owenvoke owenvoke left a comment

Choose a reason for hiding this comment

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

LGTM, is it worth using generics syntax for the doc blocks to be in line with other declarations? array<mixed> or array<int, mixed>

src/Expectation.php Outdated Show resolved Hide resolved
src/Expectation.php Outdated Show resolved Hide resolved
@alexmartinfr
Copy link
Collaborator Author

alexmartinfr commented Jul 29, 2020

I'm not familiar with doc blocks best practices yet.
Thanks for the reviews and explanations!

I'll commit this change 👍

Fixed doc blocks:

array{mixed}
to
array<mixed>
Copy link
Member

@nunomaduro nunomaduro left a comment

Choose a reason for hiding this comment

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

There are missing tests, thank you for this!

/**
* Asserts that the value array has all of the provided $keys.
*
* @param array<mixed> $keys
Copy link
Member

Choose a reason for hiding this comment

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

@param array<int, int|string> $keys.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Of course!
Forgot about the tests, I'll get on it 😌

@alexmartinfr
Copy link
Collaborator Author

alexmartinfr commented Jul 29, 2020

Writing the tests made me stumble upon a logic pitfall with the not() modifier.

When all values that shouldn't be here exist, the expected exception is thrown:

test('not - fails when all strings exist')
    ->expect('Pest is awesome')
    ->not()->toContainAll(['Pest', 'awesome'])
    ->throws(ExpectationFailedException::class);
    // OK.

However, when only parts of the condition are met, the exception is not thrown, and the test doesn't fail as it should:

test('not - fails when at least one string exists')
    ->expect('Pest is awesome')
    ->not()->toContainAll(['Pest', 'clever'])
    ->throws(ExpectationFailedException::class);
    // Should fail, but is passing.

The same problem is encountered with toHaveKeys.

I have no clue how to circle around that, any idea?

Tests for the new expectations.
WIP, some tests aren't passing yet.
@nunomaduro
Copy link
Member

@alexmartinfr You code is correct:

test('not - fail when at least one string exists')
    ->expect('Pest is awesome')
    ->not()->toContainAll(['Pest', 'clever'])
    ->throws(ExpectationFailedException::class);

Pest is awesome does NOT contains all those keys. It contains some. 👍

@nunomaduro
Copy link
Member

@alexmartinfr But maybe I expect that to contain none of the give values. Can you update the code?

@alexmartinfr
Copy link
Collaborator Author

alexmartinfr commented Jul 30, 2020

You are right @nunomaduro, not->toContainAll() is working correctly, thanks to the word all.

I should have mentioned the real issue, not->toHaveKeys():

test('not - fails when at least one key exists')
    ->expect(['a' => 1, 'b', 'c' => 'world'])
    ->not()->toHaveKeys(['hello', 'a'])
    ->throws(ExpectationFailedException::class);

    // An exception should be thrown for 'a'.
    // However it's not the case.

I've been scratching my head around this one and can't find a way to make it work as I intend.

I could rename the expectation to toHaveAllKeys(), so that it matches the actual behaviour, but it doesn't seem ideal.


Another strategy to reach my initial goal would be to have specific expectations leveraging PHPUnit's negative assertions for these purposes:

  • toContainNone() would use assertNotContains & assertStringNotContainsString
  • toHaveNone() would use assertArrayNotHasKey

(I haven't tried that yet. Even if it works, it would need a better name for the second one.)

@nunomaduro
Copy link
Member

Can we keep this on old @alexmartinfr? I will get back to it this month.

@alexmartinfr
Copy link
Collaborator Author

Sure!

@nunomaduro
Copy link
Member

Going to hold this for now, we maybe re-open this pull request later.

@nunomaduro nunomaduro closed this Aug 25, 2020
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

4 participants