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

Feat: Add a "toPassAny" expectation method #1286

Open
wants to merge 1 commit into
base: 3.x
Choose a base branch
from

Conversation

KorvinSzanto
Copy link

@KorvinSzanto KorvinSzanto commented Oct 2, 2024

What:

  • New Feature

Description:

Add a toPassAny expectation to allow for effective ->or branches:

// Succeeds because the second test doesn't throw
expect(1)->toPassAny(
    fn($e) => $e->toBeGreaterThan(10),
    fn($e) => $e->toBeLessThan(5),
);

// Fails because all tests fail
expect(new \Exception())->toPassAny(
    fn($e) => $e->toBeInstanceOf(Foo::class),
    fn($e) => $e->toBeInstanceOf(Baz::class),
    fn($e) => $e->toBeInstanceOf(Bar::class),
);

For example if I want to make sure an array only contains (InterfaceA & InterfaceB) | OtherBaseClass

expect($array)->each()->toPassAny(
    fn($e) => $e->toBeInstanceOf(A::class) && $e->toBeInstanceOf(B::class),
    fn($e) => $e->toBeInstanceOf(Foo::class),
);

Open questions

Should this be it's own Expectation type?

I don't believe so, the functionality toPassAny provides is fully encapsulated and doesn't have any effect on later expectation calls.

Should there be a special exception type that outputs all of the failures instead of just the first?

If we have an exception that outputs all the failures, we run the risk of outputting very long failures when a lot of branches are provided.

If we have an exception that is generic and doesn't output any of the failures, we make it hard to debug issues folks run into when tests fail.

*
* @throws AssertionFailedError Rethrows first caught exception on failure
*/
public function toPassAny(\Closure ...$tests): Expectation
Copy link

@dshafik dshafik Oct 4, 2024

Choose a reason for hiding this comment

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

I'd like to recommend toBeOneOf(), and to allow non-closure values that will be converted to toBe() assertions. This allows for:

expect($foo)->toBeOneOf(1, 2, 3);

Here's the code:

public function toBeOneOf(mixed ...$tests): Expectation {
    $firstException = null;
    if ($tests === []) {
        return $this;
    }

    foreach ($tests as $test) {
        if (!($test instanceof \Closure)) {
            $test = fn ($expectation) => $expectation->toBe($test);
        }

        try {
            $test(new Expectation($this->value));

            return $this;
        } catch (AssertionFailedError $e) {
            $firstException ??= $e;
        }
    }

    /** @var AssertionFailedError $firstException */
    throw $firstException;
}

Copy link
Author

@KorvinSzanto KorvinSzanto Oct 4, 2024

Choose a reason for hiding this comment

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

To me "toBeOneOf" both implies an exact count and hides the fact that any expectation could be provided.

I also don't like falling back to a default expectation because in my opinion it makes the behavior confusing when your array might contain values that are closures. Id prefer an additional ->toBeAny(...) that does something like return $this->toPassAny(array_map(fn($i) => fn($e) => $e->toBe($i), $items));

Copy link

Choose a reason for hiding this comment

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

toBeAny is good, or toBeAnyOf().

You shouldn't be passing an array, it's variadic. Your example above should spread the array as args (… array_map(…)) or it won't work with any of the code…

Copy link

Choose a reason for hiding this comment

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

I actually like the idea of "toBeOneOf" as you say the name implies, an xor type deal. I wrote it and then ditched it but it sounds more useful with that name

Copy link
Author

Choose a reason for hiding this comment

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

Yeah you'd need to spread it. Not the easiest code snippet to write on mobile 🙂

@dshafik dshafik mentioned this pull request Oct 6, 2024
2 tasks
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