Test using `$this->mock(Request::class)` always returning empty GET request #37789

trovster · 2021-06-23T15:05:53Z

Laravel Version: 8.48.1
PHP Version: 7.4.13

Description:

In a Unit test for a Controller I mocked the request using the following;

$request = $this->mock(Request::class, static function (MockInterface $mock) use ($valid): void {
    $mock->shouldReceive('validated')->once()->andReturn($valid);
});

Which gave a valid result until updating Laravel from 8.47 to 8.48.

It seems that the introduction of the singletonInstance in the commit bind mock instances as singletons so they are not overwritten and the use of that in the mock(), partialMock() and spy() methods of the InteractsWithContainer trait. This seems to mean that an empty GET request is always used.

I have been able to resolve my failing tests by going back to the longer version of the code to mock an object, as found in the documentation.

$request = $this->instance(
    Request::class,
    Mockery::mock(Request::class, static function (MockInterface $mock) use ($valid): void {
        $mock->shouldReceive('validated')->once()->andReturn($valid);
    })
);

Steps To Reproduce:

$request = $this->mock(Request::class, static function (MockInterface $mock) use ($valid): void {
    $mock->shouldReceive('validated')->once()->andReturn($valid);
});

returns different $request compared to

$request = $this->instance(
    Request::class,
    Mockery::mock(Request::class, static function (MockInterface $mock) use ($valid): void {
        $mock->shouldReceive('validated')->once()->andReturn($valid);
    })
);

The text was updated successfully, but these errors were encountered:

driesvints · 2021-06-24T08:55:48Z

This is indeed caused by #37746 which was sent in to address #37706. I can indeed see this being problematic now.

I got a different solution for the mocking problem here: #37706 (comment). But not sure if that's the path we want to go into.

Ping @Nacoma @osteel @donnysim

osteel · 2021-06-24T10:51:04Z

@driesvints yeah you'd probably want some sort of abstracted Laravel mock interface instead of wiring the Mockery one to the Laravel container

taylorotwell · 2021-06-24T14:33:58Z

Well, I'm a little confused it ever behaved any other way. You mocked the request? Why would the framework not use the mock you specified? Is there a reason you are mocking the request at all instead of just making a request using $this->get() or $this->post(), etc?

taylorotwell · 2021-06-24T14:34:31Z

Can you please share your entire test as well as the controller?

trovster · 2021-06-24T14:55:37Z

Using $this->instance(...) directly continues to work how I expect it to, with the mocked request. I am mocking the request, as I am testing a simple controller in a Unit test, so didn't want to make the full request using $this->get().

When I was investigating this, I was simply getting back an empty GET request. I am guessing this gets setup somewhere, and the singleton then just re-uses whenever you make another Request object. This was happening if I also used Request::create('/example', 'POST', ['key' => 'value']).

driesvints · 2021-06-24T14:56:33Z

@trovster can you please provide the code to reproduce this?

Nacoma · 2021-06-24T21:51:17Z

The issue can be reproduced like so:

$this->app->instance('request', Request::create('/example'));

$this->mock(Request::class, function (MockInterface $mock) {

});

// works
$this->assertInstanceOf(MockInterface::class, resolve(Request::class));
// fails
$this->assertInstanceOf(MockInterface::class, resolve('request'));

This version works in both cases:

$this->app->instance('request', Request::create('/example'));

$this->mock('request', function (MockInterface $mock) {

});

$this->assertInstanceOf(MockInterface::class, resolve(Request::class)); // works
$this->assertInstanceOf(MockInterface::class, resolve('request'));

The 'request' alias is not rebound when the new singleton is registered. The default request that's bound in SetRequestForConsole is bound to the 'request' alias but it is not overridden when rebinding.

Nacoma · 2021-06-24T22:03:22Z

I'm unsure if this is the correct approach for solving this. However, it does resolve this particular issue - assuming that my reproduction (above) is accurate.

…7810) * ensure alias is rebound when mocking items in the container (issue #37789) * fix style

taylorotwell · 2021-06-25T13:22:03Z

Will be fixed here: #37810

trovster · 2021-06-28T07:23:04Z

@Nacoma that is actually an entirely different bug you have solved. I will write a test case.

mpyw · 2021-06-28T10:40:33Z

In contrary v8.48.1 → v8.48.2 breaks my test...

    public function testInitializationForMailables(): void
    {
        $this->mock(Mailer::class)->shouldReceive('send');

        // [7.x] Multiple Mailers Per App
        // https://github.com/laravel/framework/pull/31073
        if (interface_exists(MailerFactory::class)) {
            $this->mock(MailerFactory::class)
                ->shouldReceive('mailer')
                ->andReturn($this->app->make(Mailer::class));
        }

        DB::connection();

        $this->assertFalse($this->getRecordsModifiedViaReflection());

        Mail::send(new GeneralMailable());

        // ...
}

driesvints · 2021-06-28T11:46:08Z

@mpyw you're misusing that it seems. You need to provide a callback with the assertions so they're bound to the container: https://laravel.com/docs/8.x/mocking#mocking-objects

mpyw · 2021-06-28T12:05:06Z

@driesvints In short, the following fix doesn't work.

$this->mock(Mailer::class, function (MockInterface $mock) {
    $mock->shouldReceive('send');
});

// [7.x] Multiple Mailers Per App
// https://github.com/laravel/framework/pull/31073
if (interface_exists(MailerFactory::class)) {
    $this->mock(MailerFactory::class, function (MockInterface $mock) {
        $mock
            ->shouldReceive('mailer')
            ->andReturn($this->app->make(Mailer::class));
    });
}

As you may know, Mockery::mock() has many signatures. The following usages are both valid.

Mockery::mock(string $class): MockInterface
Mockery::mock(string $class, Closure $mockDefiner): MockInterface

And InteractsWithContainer::mock() implementation looks like this:

protected function singletonInstance($abstract, $instance)
{
    $abstract = $this->app->getAlias($abstract);

    $this->app->singleton($abstract, function () use ($instance) {
        return $instance;
    });

    // Returns second argument. So this returns MockInterface.
    return $instance;
}

protected function mock($abstract, Closure $mock = null)
{
    // This also returns MockInterface.
    return $this->singletonInstance($abstract, Mockery::mock(...array_filter(func_get_args())));
}

So my original usage is valid.

driesvints · 2021-06-28T12:19:55Z

@mpyw I'm sorry but I still believe you're not correctly using that. Please try a support channel.

mpyw · 2021-06-28T12:22:48Z

@Nacoma

#37789 (comment)

I'm unsure if this is the correct approach for solving this. However, it does resolve this particular issue - assuming that my reproduction (above) is accurate.

I'll appreciate if you investigate my issue when you have a moment. Thanks

trovster · 2021-06-28T13:22:05Z

This is a simple custom Form Request;

<?php

namespace App\Http\Requests;

use Illuminate\Foundation\Http\FormRequest;

class TestRequest extends FormRequest
{
    public function rules(): array
    {
        return [
            'key' => [
                'required',
                'string',
            ],
        ];
    }
}

The following test was run using Laravel 8.47.0. php artisan test --group=mock

<?php

namespace Tests\Unit;

use App\Http\Requests\TestRequest;
use Illuminate\Routing\Controller as BaseController;
use Mockery;
use Mockery\MockInterface;
use Tests\TestCase as BaseTestCase;

/** @group mock */
class MockSingletonTest extends BaseTestCase
{
    /** @test */
    public function usingInstance(): void
    {
        $data = [
            'key' => $this->faker->word,
        ];

        /** @var \App\Http\Requests\TestRequest $request */
        $request = $this->instance(
            TestRequest::class,
            Mockery::mock(TestRequest::class, static function (MockInterface $mock) use ($data): void {
                $mock->shouldReceive('validated')->andReturn($data);
            })
        );

        $controller = $this->controller();
        $response = $this->app->call($controller, [
            $request,
        ]);

        $this->assertSame('8.47.0', $this->app->version());
        $this->assertSame($data, $request->validated());
        $this->assertSame($data, $response);
    }

    /**
     * @test
     * This test throws \Illuminate\Validation\ValidationException
     * It should work the same way as above.
    */
    public function usingPartialMock(): void
    {
        $data = [
            'key' => $this->faker->word,
        ];

        /** @var \App\Http\Requests\TestRequest $request */
        $request = $this->partialMock(
            TestRequest::class,
            static function (MockInterface $mock) use ($data): void {
                $mock->shouldReceive('validated')->andReturn($data);
            }
        );

        $controller = $this->controller();
        $response = $this->app->call($controller, [
            $request,
        ]);

        $this->assertSame('8.47.0', $this->app->version());
        $this->assertSame($data, $request->validated());
        $this->assertSame($data, $response);
    }

    protected function controller(): BaseController
    {
        return new class extends BaseController {
            public function __invoke(TestRequest $request): array
            {
                return $request->validated();
            }
        };
    }
}

PASS Tests\Unit\MockSingletonTest
✓ using instance
✓ using partial mock

Tests: 2 passed
Time: 0.46s

I then updated to Laravel 8.48.2 and re-run the test (changing the version number). php artisan test --group=mock

<?php

namespace Tests\Unit;

use App\Http\Requests\TestRequest;
use Illuminate\Routing\Controller as BaseController;
use Mockery;
use Mockery\MockInterface;
use Tests\TestCase as BaseTestCase;

/** @group mock */
class MockSingletonTest extends BaseTestCase
{
    /** @test */
    public function usingInstance(): void
    {
        $data = [
            'key' => $this->faker->word,
        ];

        /** @var \App\Http\Requests\TestRequest $request */
        $request = $this->instance(
            TestRequest::class,
            Mockery::mock(TestRequest::class, static function (MockInterface $mock) use ($data): void {
                $mock->shouldReceive('validated')->andReturn($data);
            })
        );

        $controller = $this->controller();
        $response = $this->app->call($controller, [
            $request,
        ]);

        $this->assertSame('8.48.2', $this->app->version());
        $this->assertSame($data, $request->validated());
        $this->assertSame($data, $response);
    }

    /**
     * @test
     * This test throws \Illuminate\Validation\ValidationException
     * It should work the same way as above.
    */
    public function usingPartialMock(): void
    {
        $data = [
            'key' => $this->faker->word,
        ];

        /** @var \App\Http\Requests\TestRequest $request */
        $request = $this->partialMock(
            TestRequest::class,
            static function (MockInterface $mock) use ($data): void {
                $mock->shouldReceive('validated')->andReturn($data);
            }
        );

        $controller = $this->controller();
        $response = $this->app->call($controller, [
            $request,
        ]);

        $this->assertSame('8.48.2', $this->app->version());
        $this->assertSame($data, $request->validated());
        $this->assertSame($data, $response);
    }

    protected function controller(): BaseController
    {
        return new class extends BaseController {
            public function __invoke(TestRequest $request): array
            {
                return $request->validated();
            }
        };
    }
}

FAIL Tests\Unit\MockSingletonTest
✓ using instance
⨯ using partial mock

Tests: 1 failed, 1 passed
Time: 0.47s

The error is as below;

  • Tests\Unit\MockSingletonTest > using partial mock
   Illuminate\Validation\ValidationException 

  The given data was invalid.

  at vendor/laravel/framework/src/Illuminate/Foundation/Http/FormRequest.php:137
    133▕      * @throws \Illuminate\Validation\ValidationException
    134▕      */
    135▕     protected function failedValidation(Validator $validator)
    136▕     {
  ➜ 137▕         throw (new ValidationException($validator))
    138▕                     ->errorBag($this->errorBag)
    139▕                     ->redirectTo($this->getRedirectUrl());
    140▕     }
    141▕ 

      +16 vendor frames 
  17  tests/Unit/MockSingletonTest.php:60
      Illuminate\Container\Container::call(Object(class@anonymous/tests/Unit/MockSingletonTest.php:70$21b))

mpyw · 2021-06-28T14:54:14Z

I got it!

@Nacoma's fix is still correct.
We can't mock drivers delegated by manager's magic methods and macros.
- In my previous test, MailerFactory was not correctly mocked.

// Without Mocking
Mail::send($mailable);
↓
$mailManager = Mailer::getFacadeRoot();
$mailManager->send($mailable);
↓
$mailManager = Mailer::getFacadeRoot();
$mailManager->__call('send', [$mailable]);
↓
$mailManager = Mailer::getFacadeRoot();
$mailManager->mailer()->send($mailable);

// With Mocking
Mail::send($mailable);
↓
$mailManagerMock = Mailer::getFacadeRoot();
$mailManagerMock->send($mailable);
↓
// Mockery can't find `Mailer::send()` method under `MailManager::__call()` delegation.

How do we get correct approach (not workarounds) for this problem...? @taylorotwell

trovster · 2021-07-02T08:40:35Z

Is this ticket no longer being looked at? It was closed before I provided my test case and the merged code actually solved a different bug/inconsitency.

driesvints · 2021-07-02T09:10:51Z

I'll re-open this to have another look.

driesvints · 2021-07-02T16:41:11Z

I can't figure it out. I'm just going to send in a PR to revert the PR that's causing this.

driesvints · 2021-07-02T16:48:24Z

We're tagging a new release for this.

Nacoma · 2021-07-06T20:41:37Z

@trovster @driesvints

The current example is essentially doing this:

$this->app->call($controller);

The call method attempts to bind parameters using reflection. Because the example is not passing an associative array, the container attempts to create a new request instance from the container. The $request that is being passed is here is 100% ignored. The fix for this is to name the arguments to avoid collisions with the container:

$this->app->call($controller, [
    'request' => $request,
]);

This will prevent the request object from being resolved out of the container.

If it's important to resolve the test object out of the container (for the purpose of what's being tested), there's another underlying problem with the way this is being setup.

Under non-test circumstances, FormRequest is not bound to the container as an instance. Binding it as an instance short circuits the natural resolution process and skips over the afterResolving callbacks. The afterResolving callback is where the validation is actually performed (FormRequestServiceProvider). The Illuminate\Http\Request object does not contain the correct data which causes the validation step to fail. This is the error mentioned previously.

In order to resolve the FormRequest from the container, in similar conditions to how it would be resolved in an application, one of the following must happen:

The Illuminate\Http\Request object must be bound with valid data
The TestRequest must be bound with valid data
The TestRequest mock must mock the validation that happens during the callback

Edit: Sorry it took so long to get back on this. Was away.

driesvints · 2021-07-08T12:11:00Z

Thanks @Nacoma. I'm gonna let this one rest. Anyone can attempt a new PR if they want which isn't breaking.

driesvints added the bug label Jun 24, 2021

Nacoma mentioned this issue Jun 24, 2021

[8.x] Ensure alias is rebound when mocking items in the container #37810

Merged

taylorotwell closed this as completed in #37810 Jun 25, 2021

taylorotwell pushed a commit that referenced this issue Jun 25, 2021

[8.x] Ensure alias is rebound when mocking items in the container (#3…

ef420d1

…7810) * ensure alias is rebound when mocking items in the container (issue #37789) * fix style

mpyw mentioned this issue Jun 28, 2021

Test fails due to changes in v8.48.1 → v8.48.2 mpyw/laravel-cached-database-stickiness#22

Closed

jasonvarga mentioned this issue Jun 28, 2021

Use partial mock to fix breadcrumbs test statamic/cms#3915

Merged

mpyw mentioned this issue Jun 28, 2021

fix: 🐛 Mock Swift_Mailer instead of Mail\Mailer mpyw/laravel-cached-database-stickiness#23

Merged

driesvints reopened this Jul 2, 2021

driesvints mentioned this issue Jul 2, 2021

[8.x] Revert mocking #37892

Merged

taylorotwell closed this as completed in #37892 Jul 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test using `$this->mock(Request::class)` always returning empty GET request #37789

Test using `$this->mock(Request::class)` always returning empty GET request #37789

trovster commented Jun 23, 2021

driesvints commented Jun 24, 2021

osteel commented Jun 24, 2021

taylorotwell commented Jun 24, 2021 •

edited

Loading

taylorotwell commented Jun 24, 2021

trovster commented Jun 24, 2021

driesvints commented Jun 24, 2021

Nacoma commented Jun 24, 2021 •

edited

Loading

Nacoma commented Jun 24, 2021

taylorotwell commented Jun 25, 2021

trovster commented Jun 28, 2021

mpyw commented Jun 28, 2021

driesvints commented Jun 28, 2021

mpyw commented Jun 28, 2021 •

edited

Loading

driesvints commented Jun 28, 2021

mpyw commented Jun 28, 2021

trovster commented Jun 28, 2021

mpyw commented Jun 28, 2021

trovster commented Jul 2, 2021 •

edited

Loading

driesvints commented Jul 2, 2021

driesvints commented Jul 2, 2021

driesvints commented Jul 2, 2021

Nacoma commented Jul 6, 2021 •

edited

Loading

driesvints commented Jul 8, 2021

Test using $this->mock(Request::class) always returning empty GET request #37789

Test using $this->mock(Request::class) always returning empty GET request #37789

Comments

trovster commented Jun 23, 2021

Description:

Steps To Reproduce:

driesvints commented Jun 24, 2021

osteel commented Jun 24, 2021

taylorotwell commented Jun 24, 2021 • edited Loading

taylorotwell commented Jun 24, 2021

trovster commented Jun 24, 2021

driesvints commented Jun 24, 2021

Nacoma commented Jun 24, 2021 • edited Loading

Nacoma commented Jun 24, 2021

taylorotwell commented Jun 25, 2021

trovster commented Jun 28, 2021

mpyw commented Jun 28, 2021

driesvints commented Jun 28, 2021

mpyw commented Jun 28, 2021 • edited Loading

driesvints commented Jun 28, 2021

mpyw commented Jun 28, 2021

trovster commented Jun 28, 2021

mpyw commented Jun 28, 2021

trovster commented Jul 2, 2021 • edited Loading

driesvints commented Jul 2, 2021

driesvints commented Jul 2, 2021

driesvints commented Jul 2, 2021

Nacoma commented Jul 6, 2021 • edited Loading

driesvints commented Jul 8, 2021

Test using `$this->mock(Request::class)` always returning empty GET request #37789

Test using `$this->mock(Request::class)` always returning empty GET request #37789

taylorotwell commented Jun 24, 2021 •

edited

Loading

Nacoma commented Jun 24, 2021 •

edited

Loading

mpyw commented Jun 28, 2021 •

edited

Loading

trovster commented Jul 2, 2021 •

edited

Loading

Nacoma commented Jul 6, 2021 •

edited

Loading