ellipsesynergie / api-response

Simple package to handle response properly in your API.
MIT License
377 stars 53 forks source link

Automated test responses don't include relationships #31

Closed imjohnbon closed 7 years ago

imjohnbon commented 7 years ago

In my application using this package (latest), I have an automated test that uses Laravel's (5.3 latest) built-in helpers to test a URL like this:

$this->json('GET', "api/users/{$user->id}?include=posts");

It is supposed to give me back a list of users with their posts. When I test the URL in postman or chrome, it works just fine. However, when I run the test on the command line (by running "phpunit") and dump-and-die the output, it just includes the user's information without the "posts" include.

This is happening across the board for me in my automated tests for any endpoint that has a relationship. Based on what I've found by digging into Fractal and dump-and-dying all over the place, it looks like for some reason the "parseIncludes" function doesn't get set correctly when automated tests are run. But like I said, it's fine in a Postman test.

To be honest, I can't tell for sure if this is Laravel's problem, Fractal's problem, or this packages problem. I am continuing to dig into it.

imjohnbon commented 7 years ago

I temporarily switched to https://github.com/spatie/laravel-fractal to see if the issue would still persist, and it did. Which causes me to believe this may not be an issue with this package, and rather with Laravel or Fractal instead.

This is completely perplexing me at this point. I can't imagine why this would work in practice but fail during automated tests. Very odd. Would love to hear from the creator of this package to see if he ever successfully application tested relationships.

maximebeaudoin commented 7 years ago

I'll try to take a look at it. Thank you for the report

maximebeaudoin commented 7 years ago

@imjohnbon I found the problem, but not the solution for now.

The problem is simple, it's because the parsing is triggered on the boot method of the service provider like this : $manager->parseIncludes(explode(',', $this->app['Illuminate\Http\Request']->get('include'))); So at this point, when the http request occur, the content of the request is populate with the include value.

BUT, in the testing process, we are not doing the normal http request. Laravel doing a internal call via $this->call and the value of includes was already loaded and will NOT be refresh.

For now, the only way i see to resolve this is to replace the method in Illuminate\Foundation\Testing\Concerns\MakesHttpRequests::call by this code

    /**
     * Call the given URI and return the Response.
     *
     * @param  string  $method
     * @param  string  $uri
     * @param  array   $parameters
     * @param  array   $cookies
     * @param  array   $files
     * @param  array   $server
     * @param  string  $content
     * @return \Illuminate\Http\Response
     */
    public function call($method, $uri, $parameters = [], $cookies = [], $files = [], $server = [], $content = null)
    {
        $kernel = $this->app->make('Illuminate\Contracts\Http\Kernel');

        $this->currentUri = $this->prepareUrlForRequest($uri);

        $this->resetPageContext();

        $symfonyRequest = SymfonyRequest::create(
            $this->currentUri, $method, $parameters,
            $cookies, $this->filterFiles($files), array_replace($this->serverVariables, $server), $content
        );

        $request = Request::createFromBase($symfonyRequest);

        $response = app(\EllipseSynergie\ApiResponse\Contracts\Response::class);

        $response
            ->getManager()
            ->parseIncludes(explode(',', $request->get('include')));

        $response = $kernel->handle($request);

        $kernel->terminate($request, $response);

        return $this->response = $response;
    }

This code will intercept the new Request object and parse the include value.

Someday i may probably create some Trait to help people resolve that issue.

imjohnbon commented 7 years ago

Thanks for the update! I figured it was something like that. Unfortunately editing core Laravel files isn't an option for me or most people I would assume. Hopefully one day this can be solved within the package.

maximebeaudoin commented 7 years ago

@imjohnbon You just have to create your own call method in the TestCase class for now if you want a quick fix.

imjohnbon commented 7 years ago

Ah of course, thanks for the tip!

mehrdad-shokri commented 7 years ago

@maximebeaudoin can provide a code snippet on hwo to create m own call method in TestCase

maximebeaudoin commented 7 years ago

@mehrdaad I have create a trait to resolve this. https://github.com/ellipsesynergie/api-response/blob/master/src/Testing/Laravel/AddTestingSupportForInclude.php

mehrdad-shokri commented 7 years ago

@maximebeaudoin thanks, I had a suggestion for this trait, Could we convert call to a private function and create 3 or 4 protected functions like get, post etc to keep Laravel signature untouched?
One problem I can think of it that it would create confusion for the trait client.
Any suggestions?