Skip to content
This repository has been archived by the owner on Jun 16, 2021. It is now read-only.

Add support to dot notation in toHaveKey expectation #15

Closed
wants to merge 10 commits into from
Closed

Add support to dot notation in toHaveKey expectation #15

wants to merge 10 commits into from

Conversation

fabio-ivona
Copy link

This PR wants to add support for dot notation when giving the array key to test with expect()->toHaveKey() expectation

this will allow to write a test like this one:

it('has nested key', function () {
    $array = [
        'a' => 1,
        'b' => 2,
        'c' => [
            'd' => 3,
        ],
        'e.f' => 4,
    ];
    expect($array)->toHaveKey('c.d');
    expect($array)->toHaveKey('c.d', 3);
    expect($array)->toHaveKeys(['c.d', 'a']);
    expect($array)->toHaveKey('e.f');
});

As you can see in the last expectation of the example, In order to not be a breaking change, this implementation still allows to address an array key containing a dot in the string.

some credits:

  • to parse dot notated keys I added an helper Arr static class, taking inspiration for has() and get() methods implementation from the laravel/framework project

  • @olivernybroe gave me the nice idea of allowing arrays with dots in it to obtain a non-breaking change

bonus: while adding these new tests cases to the tests/Expect/toHaveKey.php tests, I noticed that the second assertion of toHaveKey($key, $value) expectation (assert that the array has that specific value for the given key) was never tested, so added tests for it as well

Copy link
Member

@olivernybroe olivernybroe left a comment

Choose a reason for hiding this comment

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

Looks amazing and simple, just had some minor comments 👍

src/Expectation.php Outdated Show resolved Hide resolved
src/Helpers/Arr.php Outdated Show resolved Hide resolved
src/Helpers/Arr.php Show resolved Hide resolved
@fabio-ivona
Copy link
Author

Just updated code to address @olivernybroe comments

Copy link
Member

@olivernybroe olivernybroe left a comment

Choose a reason for hiding this comment

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

Really nice, could we add a test for the error message also, in case someone tries to refactor it to assertTrue with a custom message, but doesn't realise that it adds two lines 👍

Copy link
Member

@olivernybroe olivernybroe left a comment

Choose a reason for hiding this comment

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

LGTM!

Thanks for the great work ❤️

@fabio-ivona
Copy link
Author

Done, I updated failures tests adding the exceptionMessage check:

->throws(ExpectationFailedException::class, "Failed asserting that an array has the key 'foo'")

also I refactored the whole test file, using a dataset would not have been compatible with the ->throws(...) directive, as the message would change for each dataset

@fabio-ivona
Copy link
Author

hmm, seems there is an issue with composer dependencies.. is it something I can solve myself?

@olivernybroe
Copy link
Member

hmm @owenvoke could you help out with the composer issues that @fabio-ivona is having? 😃

@owenvoke
Copy link
Member

Oh, it's the old circular dependencies issue. 😦 I'm not actually sure how to resolve that. I think I opened PRs a while back that helped fix it by basically adding COMPOSER_ROOT_VERSION: 1.x to the workflow environment, but not sure where that went.

I'll open a PR in a bit to see if I can get it fixed.

@lukeraymonddowning
Copy link
Member

@owenvoke I've fixed it in #16

@nunomaduro
Copy link
Member

This tests issue will be fixed once we merge all plugins on the core. @owenvoke Is taking care of that.

@fabio-ivona
Copy link
Author

That's fine for me

At this point I don't think this PR needs more work from me, let me know if you need something else ;)

@nunomaduro
Copy link
Member

@lukeraymonddowning Can you please perform this pull request on the core?

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

Successfully merging this pull request may close these issues.

5 participants