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(testing): accept type-hinted requests as assertSent parameters #424

Open
wants to merge 2 commits into
base: v3
Choose a base branch
from

Conversation

innocenzi
Copy link

Closes #417

This pull request adds support for type-hinting a request class as the first parameter of the closure passed to MockClient#assertSent. This allows running this kind of test:

MockClient::getGlobal()->assertSent(function (CreateDealRequest $request) {
    expect($request->body()->all())->toMatchArray([
        'status' => DealStatus::OPEN,
    ]);
});

Before this pull request, the test above could only be written using a condition:

MockClient::getGlobal()->assertSent(function (Request $request) {
    if ($request instanceof CreateDealRequest) {
        expect($request->body()->all())->toMatchArray([
            'status' => DealStatus::OPEN,
        ]);

        return true;
    }
});

@innocenzi
Copy link
Author

I ignored the phpstan errors as they seem to be false-positives.

@juse-less
Copy link
Contributor

For some reason, I can't get GitHub to let me comment on the changes on my tablet, so I'll make a regular comment for now.

The PHPStan errors around the Reflection APIs might need an extra look.
F.ex., the ReflectionFunction only accepts a Closure, but we're passing a callable.
I'm not sure how common it is to pass an invokable object, though.
That should be relatively easy to fix.

new ReflectionFunction($callable(...));

I'm not 100 % sure on the ReflectionType for the parameters.
ReflectionType itself doesn't have a getName(), nor does it's children except for the ReflectionNamedType.
This may or may not be an issue.
If only one type is defined, it'll work.
If no parameters are defined, then index 0 will also give an error.
But realistically, I feel like these are edge cases.

Copy link
Member

@Sammyjo20 Sammyjo20 left a comment

Choose a reason for hiding this comment

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

The PR looks okay to me. I don't think we need these PHPStan exclusions?

Comment on lines +57 to +71

-
message: "#^Method Saloon\\\\Http\\\\Faking\\\\MockClient\\:\\:getRequestClass\\(\\) should return class-string|null but returns class-string<Saloon\\\\Http\\\\Request>|Saloon\\\\Http\\\\Request$#"
count: 1
path: src/Http/Faking/MockClient.php

-
message: "#^Call to an undefined method ReflectionType\\:\\:getName\\(\\)\\.$#"
count: 1
path: src/Http/Faking/MockClient.php

-
message: "#^Parameter \\#1 \\$function of class ReflectionFunction constructor expects Closure|string, callable\\(\\)\\: mixed given\\.$#"
count: 1
path: src/Http/Faking/MockClient.php
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
-
message: "#^Method Saloon\\\\Http\\\\Faking\\\\MockClient\\:\\:getRequestClass\\(\\) should return class-string|null but returns class-string<Saloon\\\\Http\\\\Request>|Saloon\\\\Http\\\\Request$#"
count: 1
path: src/Http/Faking/MockClient.php
-
message: "#^Call to an undefined method ReflectionType\\:\\:getName\\(\\)\\.$#"
count: 1
path: src/Http/Faking/MockClient.php
-
message: "#^Parameter \\#1 \\$function of class ReflectionFunction constructor expects Closure|string, callable\\(\\)\\: mixed given\\.$#"
count: 1
path: src/Http/Faking/MockClient.php

Copy link
Author

Choose a reason for hiding this comment

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

Without them, phpstan doesn't pass:

CleanShot 2024-08-05 at 02 14 13

I believe they are false-positive.

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.

Asserting a request is sent by closure parameter
3 participants