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

fix: adds failing test #409

Closed
wants to merge 1 commit into from
Closed

fix: adds failing test #409

wants to merge 1 commit into from

Conversation

nunomaduro
Copy link
Member

cc @lukeraymonddowning is this failing test normal?

@mertasan
Copy link
Contributor

mertasan commented Sep 24, 2021

I think this fails.

The fourth loop matches the first sequence item:

test('it works if the number of items in the iterable is bigger than the number of expectations', function () {
    expect([1, 2, 3, 4])
        ->sequence(
            function ($expectation) { $expectation->toBeInt()->toEqual(1); }, // 1 - 4
            function ($expectation) { $expectation->toBeInt()->toEqual(2); }, // 2
            function ($expectation) { $expectation->toBeInt()->toEqual(3); }, // 3
        );

    expect(static::getCount())->toBe(6);
});
expect([1, 2, 3, 4, 5, 6, 7, 8])
    ->sequence(
        function ($expectation) { $expectation->toBeInt()->toBeIn([1, 4, 7]); },
        function ($expectation) { $expectation->toBeInt()->toBeIn([2, 5, 8]); },
        function ($expectation) { $expectation->toBeInt()->toBeIn([3, 6]); },
    );

expect(static::getCount())->toBe(16);

@lukeraymonddowning
Copy link
Member

As originally intended, sequence will loop back over the closures until all items in the iterable had been checked. So in the case of this failing test, there would be 8 assertions, not 6, because the first expectation in the sequence would be called twice (for the first and last item in the sequence).

@fabio-ivona
Copy link
Collaborator

fabio-ivona commented Sep 25, 2021

@nunomaduro another error seems to rise in src/PendingObjects/TestCall.php:91 in test runs with php<8

I think mixed return type will work only from php8.0 onward (see here)

image

Why did you need to use mixed instead of a simple bool? wasn't $condition parameter of type bool?

@param (callable(): bool)|bool $condition

otherwise the return type on line 91 should not be defined at all (return seems to be assumed to be mixed if not defined)

this breaks the pipeline in #405 also, but I'm not fixing it there, maybe we should do it here?

@nunomaduro
Copy link
Member Author

@fabio-ivona fixed.

@lukeraymonddowning "because the first expectation in the sequence would be called twice (for the first and last item in the sequence)." This seems weird to me. I would expect the first expectation called once, on the first element of the iterable. There is a reasoning about this behaviour ?

@lukeraymonddowning
Copy link
Member

The reasoning was to match how sequence works in Laravel factories, which is loosely what this was based on.

@nunomaduro nunomaduro closed this Sep 25, 2021
@nunomaduro
Copy link
Member Author

Thank you @lukeraymonddowning

@nunomaduro nunomaduro deleted the fix/sequence-test branch September 25, 2021 13:09
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.

4 participants