Faking HTTP client endpoints in combination with disabling or mocking events clears the faked endpoints

[sebastiaanluca]

• Laravel Version: 8.82.0
• PHP Version: 8.0.14
• Database Driver & Version: N/A

Description:

Faking HTTP client requests before calling $this->withoutEvents() (or expecting an event which calls that method) will clear the faked HTTP client requests and execute real requests.

Steps To Reproduce:

A simple test case with an event assertion and a fake API call that fails (HTTP calls are not stubbed, but made to real endpoints):

Http::fake(['http://example.test/ping' => Http::response(null, Response::HTTP_NO_CONTENT)]);

$this->expectsEvents(UserUnsubscribed::class);

$this->actingAs(User::factory()->create())
    ->get(route('home'));

The same test that passes (HTTP calls are stubbed):

$this->expectsEvents(UserUnsubscribed::class);

Http::fake(['http://example.test/ping' => Http::response(null, Response::HTTP_NO_CONTENT)]);

$this->actingAs(User::factory()->create())
    ->get(route('home'));

Remarks:

So switching the calls works fine, but that's not possible in every situation or test (e.g. endpoints faked manually, in the test setUp method, or in traits) and is a blocking issue when writing our tests. It's also a gotcha and not known to most users, I assume.

I did some debugging and it creates \Illuminate\Http\Client\Factory twice, clearing the previously stubbed endpoints in its constructor ($stubCallbacks = collect()). This because, to rebind the mocked event dispatcher, it seems it needs to recreate any class that was already using the non-mocked instance (\Illuminate\Container\Container::rebound()) so it can receive the new one.

I tried to do something as in \Illuminate\Auth\AuthServiceProvider::registerEventRebindHandler() and copy over the stubbed calls from the previous HTTP factory instance to the next, but there's no way to get stubCallbacks from Factory and also not an option to manually set them. In addition, the callbacks are already cleared before it hits the rebinding callback (probably in \Illuminate\Container\Container::instance()).

/**
 * Handle the re-binding of the event dispatcher binding.
 *
 * @return void
 */
protected function registerEventRebindHandler()
{
    $this->app->rebinding('events', function ($app, $dispatcher) {
        if (! $app->resolved('auth') ||
            $app['auth']->hasResolvedGuards() === false) {
            return;
        }

        if (method_exists($guard = $app['auth']->guard(), 'setDispatcher')) {
            $guard->setDispatcher($dispatcher);
        }
    });
}

I'll try to create a demo repo later today.

[sebastiaanluca]

I set up a new repo as per the instructions and all tests pass on v9. I was able to recreate the bug on v8.82.0 however.

https://github.com/sebastiaanluca/laravel-http-client-stubbing-bug/blob/main/tests/Feature/ExampleTest.php

If you want to easily check out the repo and see the difference:

• v9: sebastiaanluca/laravel-http-client-stubbing-bug@1a7ec52
• v8: sebastiaanluca/laravel-http-client-stubbing-bug@86b9b21

[driesvints]

Sorry @sebastiaanluca, didn't get to this this week. I'll have another look next week.

To already reply a bit: I vaguely remember that this has been the case in previous Laravel versions indeed. Maybe attempt to find a PR that fixes it in v9? It could be that the PR contained breaking changes which is why it maybe didn't land in 8.x. I also think I remember other issues being posted about this so maybe also have a look at the issue tracker.

[derekmd]

Related PR: #36716

The MocksApplicationServices trait that defines methods expectsEvents() and withoutEvents() has been marked deprecated in place of fakers since the 8.x branch: https://github.com/laravel/framework/blob/8.x/src/Illuminate/Foundation/Testing/Concerns/MocksApplicationServices.php#L12

These methods are also no longer covered by the official docs: https://laravel.com/docs/8.x/mocking

I believe this was marked deprecated for the very reason of this issue's example. Dependency injection & app container inversion of control can cause unexpected results depending on calling order of services during feature test setup.

withoutEvents() invoking Event::clearResolvedInstances() purges every facade instance (not just app('events')) affecting previous facade calls. Some of those other cleared facade instances can also still be leftover bound to the app container.

Copying this override into your app's base tests/TestCase.php may allow the behavior you're looking for:

protected function withoutEvents()
{
    $mock = Mockery::mock(EventsDispatcherContract::class)->shouldIgnoreMissing();
    $mock->shouldReceive('dispatch', 'until')->andReturnUsing(function ($called) {
        $this->firedEvents[] = $called;
    });
    
    // Event::clearResolvedInstances();
    Event::clearResolvedInstance('events');
    
    $this->app->instance('events', $mock);
    
    return $this;
}

---

The Event::fake() API provides a more determinstic way to intercept exact events and check whether they fired.

// Can be place first.
// Event::fake(UserUnsubscribed::class);

Http::fake(['http://example.test/ping' => Http::response(null, Response::HTTP_NO_CONTENT)]);

// Or second, this doesn't affect other facades.
Event::fake(UserUnsubscribed::class); 

$this->actingAs(User::factory()->create())
    ->get(route('home'));

Event::assertDispatched(UserUnsubscribed::class);

---

For your example app's 8.x commit, this passes:

tests/Feature/ExampleTest.php

namespace Tests\Feature;

use Illuminate\Support\Facades\Event;
use Illuminate\Support\Facades\Http;
use Tests\TestCase;

class ExampleTest extends TestCase
{
    public function test_it_can_make_an_actual_http_request()
    {
        Event::fake('my-event');

        $response = $this->get(route('home'));

        $response->assertOk();

        $this->assertNotNull($response->getContent());

        Event::assertDispatched('my-event');
    }

    public function test_it_can_stub_http_endpoints()
    {
        Event::fake('my-event');

        Http::fake([
            'https://www.google.com/' => Http::response('stubbed'),
            '*' => Http::response('Unhandled endpoint', 404),
        ]);

        $response = $this
            ->withoutExceptionHandling()
            ->get(route('home'));

        $response->assertOk();

        $this->assertSame('stubbed', $response->getContent());

        Event::assertDispatched('my-event');
    }

    public function test_it_can_stub_http_endpoints_but_makes_actual_calls()
    {
        Http::fake([
            'https://www.google.com/' => Http::response('stubbed'),
            '*' => Http::response('Unhandled endpoint', 404),
        ]);

        Event::fake('my-event');

        $response = $this
            ->withoutExceptionHandling()
            ->get(route('home'));

        $response->assertOk();

        $this->assertSame('stubbed', $response->getContent());

        Event::assertDispatched('my-event');
    }
}

[driesvints]

Thanks @derekmd, that's what I was looking for. @sebastiaanluca please see @derekmd's response why these two can't be combined. It's probably also best that we remove that trait in Laravel v10.

[sebastiaanluca]

@derekmd Thanks for the answer and detailed explanation! Much appreciate it.

@driesvints Would've loved a deprecation warning for this 😅 I see that was mentioned in one of the comments on the PR and marked as a PHPStorm bug, but almost a year has passed and they haven't fixed it AFAIK. I anticipate this will cause quite a bit of confusion and frustration for those relying on the old methods and trait. Maybe a deprecation warning (or a proper mention in the upgrade guide) is still advised?

[driesvints]

@sebastiaanluca This will be documented in v10's upgrade guide. But tbh it's more that you were trying to combine two ways of mocking which don't work together. Definitely not saying that's your fault but we didn't document or intended that to ever work together (I now realize). Therefor, we're removing this in v10.

We can't help it that PHPStorm doesn't properly indicates a deprecated method.