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

implement findAll to allow fluent assertions against all elements that match selector #22

Merged
merged 5 commits into from
Jun 17, 2024

Conversation

FRFlor
Copy link
Contributor

@FRFlor FRFlor commented Jun 17, 2024

Purpose

Hello there 👋,

I believe that, at the moment, the way to make assertions against multiple matches is done through $assertElement->contains() and $assertElement->doesntContain() methods.

The limitation of these approaches is that the contains() doesn't allow us to write an assertion that ensures all elements under a given selector respect a specific set of rules.

For example:
"Assert all images being rendered have an 'alt' attribute"

->assertElementExists(fn(AssertElement $viewRendered) => viewRendered
    ->doesntContain('img', [???]) // Without alt attribute

I believe we cannot perform this assertion with the existing resources. Let me know if I'm mistaken.

My proposed solution is the following:

image

The reasoning behind this proposal is two-fold:

  • It allows us to make sweeping assertions about what must be enforced for all elements in the selection.
  • It allows us to continue making fluent assertions over all elements in the selection
image
  • It resembles the ->has->each() API that exists in the FluentJSON assertion
image

I hope this doesn't go against the philosophy of the package.
I'm looking forward to any feedback or suggestions.

@FRFlor FRFlor closed this Jun 17, 2024
@FRFlor FRFlor reopened this Jun 17, 2024
@sinnbeck
Copy link
Owner

Thanks for the PR!
I love the idea, but perhaps just name it each like with AssertableJson. That would make it much easier to remember :)

@FRFlor
Copy link
Contributor Author

FRFlor commented Jun 17, 2024

I love it, sounds good

Thanks for the PR! I love the idea, but perhaps just name it each like with AssertableJson. That would make it much easier to remember :)

Awesome 🎉! I'm glad you liked it!

Good point! Naming it each() makes it more descriptive.

@FRFlor
Copy link
Contributor Author

FRFlor commented Jun 17, 2024

@sinnbeck While I was updating the readme, I noticed there was a section of it using contains() with a callback being passed as an argument:

image

I think this is probably from an older version of the API. At the moment the contains does not take callbacks.

I included a quick tweak to this section to this instead:

image

But let me know if you would prefer me to revert this change. This is in the HEAD commit of the PR, I could just remove it.

@sinnbeck
Copy link
Owner

Oh nice catch! Normally I would prefer it in a separate PR but I will just release it as part of the same version :) Ill draft a new release right away

@sinnbeck sinnbeck merged commit bd882c2 into sinnbeck:main Jun 17, 2024
24 checks passed
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.

2 participants