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

Add shorthands for fake HTTP responses #53663

Merged
merged 2 commits into from
Nov 25, 2024

Conversation

jasonmccreary
Copy link
Contributor

Similar to Process::fake(), this adds shorthands when faking HTTP responses. A shorthand for passing arrays already existed. This adds an explicit test for that, plus shorthands for strings for the body or an integer for a valid HTTP status code (1xx-5xx).

Before

Http::fake([
    'google.com' => Http::response('Hello World'),
    'github.com' => Http::response(['foo' => 'bar']),
    'forge.laravel.com' => Http::response(status: 204),
]);

After

Http::fake([
    'google.com' => 'Hello World',
    'github.com' => ['foo' => 'bar'],
    'forge.laravel.com' => 204,
]);

@taylorotwell taylorotwell merged commit 7afa6b6 into laravel:11.x Nov 25, 2024
40 checks passed
@jasonmccreary jasonmccreary deleted the http-fake-shorthands branch November 25, 2024 20:36
@shaedrich
Copy link
Contributor

Interpreting any number between 100 and 600 as a status code can lead to issues when this behavior is not expected (principle of least astonishment)

@jasonmccreary
Copy link
Contributor Author

jasonmccreary commented Nov 26, 2024

@shaedrich, I might agree if this were a change to app-level code. However, this is a shorthand for faking an HTTP response in a test. At this level, the developer is in full control. Furthermore this is a new feature. So there's not chance of breaking existing code.

@shaedrich
Copy link
Contributor

I'm aware of it being new. But being new doesn't mean it cannot be confusing. Sure, you can say "if you're in full control, it's your fault if it doesn't work and it's your responsibility to figure out on your own why it doesn't work", but this just isn't following the principle of least astonishment nonetheless.

@andrey-helldar
Copy link
Contributor

What if you only need to return a number in response? For example:

Http::fake([
    'google.com' => 'Hello World',
    'github.com' => ['foo' => 'bar'],
    'forge.laravel.com' => 204,
    'example.com' => Http::response(204),
]);

In this case, the number 204 for requests to forge.laravel.com would be the code status and for example.com would be the value with a status of 200 OK.

This is a non-obvious behavior.

@jasonmccreary
Copy link
Contributor Author

@andrey-helldar, you can make it a string. This is a new shorthand. You don't have to use it. It's new behavior and has no conflict.

The following are equivalent:

Http::fake([
    'forge.laravel.com' => '204',
    'example.com' => Http::response(204),
]);

@andrey-helldar
Copy link
Contributor

andrey-helldar commented Nov 27, 2024

@jasonmccreary, you know the application context, now imagine a new developer on a project not reading the changelog for Laravel releases. And he sees the following, for example:

Http::fake([
    'foo.laravel.com' => 204,
    'bar.laravel.com' => '204',
    'baz.laravel.com' => Http::response(204),
]);

Question: how is he supposed to realize that:

'foo.laravel.com' => 204 is a 'foo.laravel.com' => Http::response(status: 204)
'bar.laravel.com' => '204' is a 'bar.laravel.com' => Http::response(body: 204)

?

That's why I say it's non-obvious behavior. It's clearly misleading.

With array and text I can still understand how and if just the number 204 would mean Http::response(body: 204), but no, 204 is the response code. That's exactly what's confusing.

When I read this code:

Http::fake([
    'google.com' => 'Hello World',
    'github.com' => ['foo' => 'bar'],
    'forge.laravel.com' => 204,
]);

I perceive and expect to see the following:

Http::fake([
    'google.com' => Http::response(body: 'Hello World'),
    'github.com' => Http::response(body: ['foo' => 'bar']),
    'forge.laravel.com' => Http::response(body: 204),
]);

But no, I would have to spend some time to understand why 204 is returned not in the body of the response with code 200, but with a response without a body and code 204. Then I'd have to spend some more time rewriting it to:

Http::fake([
    'google.com' => Http::response(body: 'Hello World'),
    'github.com' => Http::response(body: ['foo' => 'bar']),
    'forge.laravel.com' => Http::response(status: 204),
]);

I'll simplify my question: what is the difference between 204 and '204' for someone new to the project?

@jasonmccreary
Copy link
Contributor Author

jasonmccreary commented Nov 27, 2024

@andrey-helldar, if you find it misleading, you don't have to use it. This is provided as a shorthand for devs that write a lot of tests and want to save a few keystrokes. They understand the tradeoff and can educate their team. 👍

As noted in the PR, there are other shorthands for fakes within Laravel. So this isn't a new concept.

@andrey-helldar
Copy link
Contributor

andrey-helldar commented Nov 27, 2024

@jasonmccreary, I like the proposed shorthands, but not for statuses. They are misleading, IMHO :)

I would suggest for statuses to explicitly return Http::response(status: ...) and everything else as a response body.

@shaedrich
Copy link
Contributor

shaedrich commented Nov 27, 2024

@andrey-helldar I agree. The overall idea is pretty convenient and neat. Just the status code is encouraging trouble. Thanks for your explanations. They should make the problem clearer 👍🏻

@timacdonald
Copy link
Member

Just noticed this discussion. Wanted to drop in another opinion: the status code feature is lovely.

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.

5 participants