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.

Sign up for GitHub

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

[10.x] Facade Fake Awareness #46188

Merged
merged 8 commits into from
Feb 22, 2023
Merged

[10.x] Facade Fake Awareness #46188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9f615f5
Don't fake facades if they've already been faked
joelbutcher Feb 20, 2023
f2490f3
Fix auto-imported FQNs
joelbutcher Feb 20, 2023
1999073
Remove unused param
joelbutcher Feb 20, 2023
310ae09
fix test
joelbutcher Feb 20, 2023
d39e3c2
Update src/Illuminate/Support/Facades/Facade.php
joelbutcher Feb 20, 2023
4b40621
Update src/Illuminate/Support/Facades/Queue.php
joelbutcher Feb 20, 2023
b84ff7b
Update src/Illuminate/Support/Facades/Facade.php
driesvints Feb 21, 2023
6ae1d1d
Update Facade.php
taylorotwell Feb 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions src/Illuminate/Support/Facades/Bus.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,11 @@ class Bus extends Facade
*/
public static function fake($jobsToFake = [], BatchRepository $batchRepository = null)
{
static::swap($fake = new BusFake(static::getFacadeRoot(), $jobsToFake, $batchRepository));

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

/**
Expand Down
12 changes: 7 additions & 5 deletions src/Illuminate/Support/Facades/Event.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,14 @@ class Event extends Facade
*/
public static function fake($eventsToFake = [])
{
static::swap($fake = new EventFake(static::getFacadeRoot(), $eventsToFake));
return tap(new EventFake(static::getFacadeRoot(), $eventsToFake), function ($fake) {
if (! static::isFake()) {
static::swap($fake);

Model::setEventDispatcher($fake);
Cache::refreshEventDispatcher();

return $fake;
Model::setEventDispatcher($fake);
Cache::refreshEventDispatcher();
}
});
}

/**
Expand Down
14 changes: 14 additions & 0 deletions src/Illuminate/Support/Facades/Facade.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use Illuminate\Support\Arr;
use Illuminate\Support\Js;
use Illuminate\Support\Str;
use Illuminate\Support\Testing\Fakes\Fake;
use Mockery;
use Mockery\LegacyMockInterface;
use RuntimeException;
Expand Down Expand Up @@ -183,6 +184,19 @@ public static function swap($instance)
}
}

/**
* Determines whether a "fake" has been set as the facade instance.
*
* @return bool
*/
protected static function isFake()
{
$name = static::getFacadeAccessor();

return isset(static::$resolvedInstance[$name]) &&
static::$resolvedInstance[$name] instanceof Fake;
}

/**
* Get the root object behind the facade.
*
Expand Down
8 changes: 5 additions & 3 deletions src/Illuminate/Support/Facades/Mail.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,11 @@ class Mail extends Facade
*/
public static function fake()
{
static::swap($fake = new MailFake(static::getFacadeRoot()));

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

/**
Expand Down
8 changes: 5 additions & 3 deletions src/Illuminate/Support/Facades/Notification.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,11 @@ class Notification extends Facade
*/
public static function fake()
{
static::swap($fake = new NotificationFake);

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

/**
Expand Down
8 changes: 5 additions & 3 deletions src/Illuminate/Support/Facades/Queue.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,11 @@ public static function popUsing($workerName, $callback)
*/
public static function fake($jobsToFake = [])
{
static::swap($fake = new QueueFake(static::getFacadeApplication(), $jobsToFake, static::getFacadeRoot()));

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

/**
Expand Down
2 changes: 1 addition & 1 deletion src/Illuminate/Support/Testing/Fakes/BusFake.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
use Illuminate\Support\Traits\ReflectsClosures;
use PHPUnit\Framework\Assert as PHPUnit;

class BusFake implements QueueingDispatcher
class BusFake implements Fake, QueueingDispatcher
{
use ReflectsClosures;

Expand Down
2 changes: 1 addition & 1 deletion src/Illuminate/Support/Testing/Fakes/EventFake.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
use PHPUnit\Framework\Assert as PHPUnit;
use ReflectionFunction;

class EventFake implements Dispatcher
class EventFake implements Dispatcher, Fake
{
use ForwardsCalls, ReflectsClosures;

Expand Down
8 changes: 8 additions & 0 deletions src/Illuminate/Support/Testing/Fakes/Fake.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?php

namespace Illuminate\Support\Testing\Fakes;

interface Fake
{
//
}
2 changes: 1 addition & 1 deletion src/Illuminate/Support/Testing/Fakes/MailFake.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
use Illuminate\Support\Traits\ReflectsClosures;
use PHPUnit\Framework\Assert as PHPUnit;

class MailFake implements Factory, Mailer, MailQueue
class MailFake implements Factory, Fake, Mailer, MailQueue
{
use ForwardsCalls, ReflectsClosures;

Expand Down
2 changes: 1 addition & 1 deletion src/Illuminate/Support/Testing/Fakes/NotificationFake.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
use Illuminate\Support\Traits\ReflectsClosures;
use PHPUnit\Framework\Assert as PHPUnit;

class NotificationFake implements NotificationDispatcher, NotificationFactory
class NotificationFake implements Fake, NotificationDispatcher, NotificationFactory
{
use Macroable, ReflectsClosures;

Expand Down
2 changes: 1 addition & 1 deletion src/Illuminate/Support/Testing/Fakes/QueueFake.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
use Illuminate\Support\Traits\ReflectsClosures;
use PHPUnit\Framework\Assert as PHPUnit;

class QueueFake extends QueueManager implements Queue
class QueueFake extends QueueManager implements Fake, Queue
{
use ReflectsClosures;

Expand Down
4 changes: 2 additions & 2 deletions tests/Integration/Queue/UniqueJobTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,9 @@ public function testUniqueJobsAreNotDispatched()
$this->app->get(Cache::class)->lock($this->getLockKey(UniqueTestJob::class), 10)->get()
);

Bus::fake();
Bus::assertDispatchedTimes(UniqueTestJob::class);
UniqueTestJob::dispatch();
Bus::assertNotDispatched(UniqueTestJob::class);
Bus::assertDispatchedTimes(UniqueTestJob::class);
Copy link
Member

Choose a reason for hiding this comment

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

Why was this test changed?

Copy link
Contributor Author

@joelbutcher joelbutcher Feb 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Member

@taylorotwell taylorotwell Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@joelbutcher joelbutcher Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.


$this->assertFalse(
$this->app->get(Cache::class)->lock($this->getLockKey(UniqueTestJob::class), 10)->get()
Expand Down