[10.x] Facade Fake Awareness

[joelbutcher]

Description

This PR attempts to address a slight issue with Facade fakes. Currently, if a facade fake requires an instance of the root as a constructor argument. (e.g. MailFake::__construct() or BusFake::__construct()), then calling fake() more than once can cause tests to fail in userland.

Let's consider the following scenario that currently passes in 9.x:

<?php

namespace Tests

use Illuminate\Support\Facades\Mail;
use Tests\TestCase;

class ExampleTest extends TestCase
{
    public function setUp(): void
    {
        // ...
        Mail::fake();
        // ...
    }

    // ...


    public function testItWorks(): void
    {
        Mail::fake();

        // ...
    }
}

This test case runs perfectly fine on Laravel < 10.0.0, but due to the merge of #45988 and #46055 will fail in Laravel >= 10.0.0. This is because we end up with a small loop:

Mail::fake() loads Mail::getFacadeRoot() (also resolvable as app('mail.manager')) as a constructor argument of MailFake. On the first attempt, this call resolves to the correct MailManager. However on the second pass, the binding to mail.manager has been swapped out with the new MailFake instance in the container. Causing errors like this one:

Illuminate\Support\Testing\Fakes\MailFake::__construct(): Argument #1 ($manager) must be of type 
Illuminate\Mail\MailManager, Illuminate\Support\Testing\Fakes\MailFake given, called in 
/var/www/vendor/laravel/framework/src/Illuminate/Support/Facades/Mail.php on line 68

---

Solution

To solve this issue, I've created a new \Illuminate\Support\Testing\Fakes\Fake interface that is used to determine if the current facade root is being "faked". If that's the case, then in the facade itself I skip the call to static::swap(...).

I've then taken inspiration from isMock() used for determining if the facade is a mockery instance during for testing and added a new protected function isFake() which is checked inside the fake():

$fake = // ...

return tap($fake, function ($fake) {
    if (!static::isFake()) {
        static::swap($fake);

        // ...
    }
});

Facades updated

Facade	Done
Bus	✅
Event	✅
Mail	✅
Notification	✅
Queue	✅

[driesvints]

Thanks @joelbutcher

[taylorotwell]

Why was this test changed?

[joelbutcher]

@taylorotwell because of the changes made here:

// \Illuminate\Support\Facades\Bus.php

return tap(new BusFake(static::getFacadeRoot(), $jobsToFake, $batchRepository), function ($fake) {
    if (! static::isFake()) {
        static::swap($fake);
    }
});

Previously, this test:

1. Swapped the underlying facade root instance for BusFake with a $dispatcher property of the previous facade root (\Illuminate\Bus\Dispatcher) – Line 42
2. Swapped out the faked facade root for a new BusFake instance, now with a $dispatcher property of the previous BusFake instance resolved via static::getFacadeRoot() (effectively making it a "nested" instance) – Line 51.

As a result of the fix, calling Bus::fake() twice in the same test now causes Bus::assertNotDispatched(...) to fail, which is correct, as we're now using the same \Illuminate\Contracts\Bus\QueueingDispatcher instance for the BusFake::$dispatcher property.

This test is now correct because we are now correctly verifying that the unique job was dispatched exactly once on the underlying BusFake instance, rather than it actually being executed twice on two separate BusFake instances.

Generally, I would say that calling Facade::fake() twice in the same test is an incorrect use of that feature.

[taylorotwell]

I'm sorry - I can't merge this with this test change. I don't understand this test anymore and we've had too many breaking changes lately.

The test used to plainly read that a unique job was dispatched and a lock acquired, then a second job couldn't be dispatched since a lock was in place for the unique job. The test no longer reads that way and is confusing. Please feel free to mark this as ready for review again when the test reads more plainly.

[joelbutcher]

It makes more sense if you disregard the "faked" element to this test. This test has been updated to reflect the REAL functionality of the Bus facade and underlying manager regardless of the call to Bus::fake().

In userland, you're not going to call Bus::fake(), so if you called UniqueTestJob::dispatch() twice in userland, like the test, then the the expectation is that this job was not dispatched more than once.

This PR identifies a problem with using Bus::getFacadeRoot() and passing the result to BusFake – calling Bus::fake() twice means you end up with the secondBusFake instance deferring to the first BusFake instance rather than the underlying Dispatcher instance (which is what it should do). In any case, calling Bus::fake() (or any Facade::fake()) more than once in a test should be considered bad practice as it leads to incorrect behaviour where getFacadeRoot` is concerned.

If it helps readability, I can add an assertDispatchedOnce method to the BusFake class and update the test accordingly.