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

[9.x] Allow forcing requests made via the Http client to be faked #42230

Merged
merged 4 commits into from
May 3, 2022

Conversation

timacdonald
Copy link
Member

@timacdonald timacdonald commented May 3, 2022

Purpose

This PR aims to bring the ability for developers to force any requests made via the Http client to be faked. If a request is not faked, an exception will be thrown.

Why this feature is useful

This feature it useful really only in a test suite.

I've often wanted the ability to throw an exception when a request made via the Http client was not faked. This serves as a high fidelity signal to the developer that they have missed something.

I've seen it be common place in a test suite that real requests are still being made to services that should have been faked. I'm sure I've been guilty of this myself on more than one occasion.

These are often only detected when then service API is down, you hit rate limits (due to the test suite), or you notice that a specific test hangs while making the request. Or sometimes they are not "detected" and instead just retried and left to live on.

Usage

This new feature could be used in any TestCase::setUp() method.

/* ... */

use Illuminate\Support\Facades\Http;

abstract class TestCase extends BaseTestCase
{
    use CreatesApplication;

    protected function setUp(): void
    {
        parent::setUp();

        Http::enforceFaking();
    }

}

Any request made that is not "faked" will now throw an exception.

public function testItDoesTheThingWithoutFaking(): void
{
    $this->post('endpoint-that-utilises-the-http-facade');

   // RuntimeException: Attempted request to [https://acme.com] without a matching fake.

   /* ... */
}

public function testItDoesTheThingWithFaking(): void
{
    Http::fake(['https://acme.com' => Http::response('ok')]);

    $response = $this->post('endpoint-that-utilises-the-http-facade');

   // no exception thrown...

   /* ... */
}

Once this feature has been turned on it is still possible to disable it for a specific test...

public function testItDoesTheThingWithoutFaking(): void
{
    Http::dontEnforceFaking();

    $this->post('endpoint-that-utilises-the-http-facade');

   // no exception thrown...

   /* ... */
}

Naming

I'm not in love the naming here tbh. I went with "fake" rather than stub as that is mostly the wording used for the current developer facing things. Feedback welcomed.

Using existing features

Unfortunately we are unable to utilise the existing faking functionality to achieve this due to precedence.

Http::fake(Closure);

// in the test setUp...

Http::fake(fn () => throw new Exception('Need to fake.'));

// in the test...

Http::fake(['https://acme.com' => Http::response('ok')]);

Http::get('https://acme.com');

// Exception: Need to fake.

Http::fake(array);

// in the test setUp...

Http::fake(['*' => fn () => throw new Exception('Need to fake.')]);

// in the test...

Http::fake(['https://acme.com' => Http::response('ok')]);

Http::get('https://acme.com');

// Exception: Need to fake.

Further considerations

Provide a handler

It is possible to allow the developer to "handle" requests that have not been faked. I did not add this feature as I'm not sure of a use-case other than to throw an exception and didn't see any value in allowing the developer to adjust the exception thrown in the test suite.

This is why I landed on a simple "flag" approach rather than a handler approach. Would love feedback from everyone on this though.

"without" style method

A lot of the testing helpers in Laravel are provided via the $this->withoutWhatever() style methods on the TestCase. I did not add this as I think that most of the Facade faking is usually achieved on the Facade itself. Happy to add one if we feel this would be a nice addition.

Shout out to @jessarcher for all the feedback on this one.

@driesvints
Copy link
Member

Http::fakeAll()?

@timacdonald
Copy link
Member Author

timacdonald commented May 3, 2022

@driesvints So we played with this naming - but we didn't feel it conveys what is actually happening.

fakeAll() to me feels like it is saying that is it handling the faking of all the requests, similar to what Http::fake() does, where it just returns a 200 for every request made.

I wanted to convey to the dev that you will be required to fake everything yourself.

Subtle difference, but I feel an important one.

@clarkeash
Copy link
Contributor

This would be really useful. Maybe Http::preventRealRequests()

@bakerkretzmar
Copy link
Contributor

Love this and will immediately enable it everywhere 😄 what does Http::fake() return? Http::fake([...])->force() feels nice to me, although it could be easy to miss if there's lots of faking stuff going on.

@taylorotwell
Copy link
Member

Going with Http::preventStrayRequests and Http::allowStrayRequests for now. 👍

@taylorotwell taylorotwell merged commit 1ab1aa3 into laravel:9.x May 3, 2022
@timacdonald timacdonald deleted the http-facade-faking branch May 9, 2022 03:59
deevus pushed a commit to deevus/framework that referenced this pull request Nov 10, 2022
…ravel#42230)

* Allow forcing requests made via the Http client to be faked

* formatting

* remove facade level method

* refactoring

Co-authored-by: Taylor Otwell <taylor@laravel.com>
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