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

[8.x] Make assertSee, assertSeeText, assertDontSee and assertDontSeeText accept an array #34982

Merged
merged 2 commits into from
Oct 27, 2020

Conversation

johnRivs
Copy link

This PR allows assertSee, assertSeeText, assertDontSee and assertDontSeeText to work with arrays. Unlike the -InOrder methods, these won't check the order.

Before:

$this->get('/')
    ->assertSee('some')
    ->assertSee('piece')
    ->assertSee('of')
    ->assertSee('text');

After:

$this->get('/')
    ->assertSee(['some', $foo, 'text', 'piece', 'of']);

Benefits

It's mostly a code style change. In projects where the frontend can change unexpectedly, this will avoid errors because of the order while keeping things compact

Notes

  • I considered doing more type checking and throw a TypeError. The idea was to provide a bit more feedback than htmlspecialchars() expects parameter 1 to be string, _______ given but I'm not too sure about it.
  • I'm running assertStringContainsString inside a foreach because I don't know if there's a better PHPUnit assertion for this case. This makes it so OK (1 test, 3 assertions) shows several assertions (one for each item in the array) which never bothered me. I'm guessing copying the strategy used in Illuminate\Testing\Constraints\SeeInOrder could deal with this?
  • First PR here. Open to any correction or suggestion.

@taylorotwell
Copy link
Member

You can make your code simpler by just always using Arr::wrap($value) on the passed argument so that you can always loop over it and not have to check is_array.

@johnRivs
Copy link
Author

Ahh, I thought about that but didn't know about Arr::wrap so I discarded the idea thinking I had to do the check anyway.

@taylorotwell taylorotwell merged commit 56bf628 into laravel:8.x Oct 27, 2020
@johnRivs
Copy link
Author

Earlier I saw I was able to add commits to the PR. I'm guessing now that it's merged I'd have to open a new one, right? (It's for the Arr::wrap cleanup)

@driesvints
Copy link
Member

@johnRivs yeah a new PR will be needed if this one is already merged.

@johnRivs
Copy link
Author

Turns out Taylor already cleaned it up haha Thanks, both of ya!

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.

3 participants