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.

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

Functional tests fail when NotFoundHttpException is thrown #25311

Closed
gjuric opened this issue Dec 4, 2017 · 15 comments
Closed

Functional tests fail when NotFoundHttpException is thrown #25311

gjuric opened this issue Dec 4, 2017 · 15 comments

Comments

@gjuric
Copy link
Contributor

gjuric commented Dec 4, 2017

Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version 4.0.0

When I am running functional tests that extend WebTestCase and I request a controller action that throws a NotFoundHttpException it looks like the exception is not converted to u 404 response. (I am checking for a 404 response code in the test).

#!/usr/bin/env php
PHPUnit 6.5.2 by Sebastian Bergmann and contributors.

Testing Project Test Suite
E                                                                   1 / 1 (100%)

Time: 275 ms, Memory: 8.00MB

There was 1 error:

1) App\Tests\DefaultControllerTest::testSomething
Symfony\Component\HttpKernel\Exception\NotFoundHttpException: Not Found

...sf4/vendor/symfony/framework-bundle/Controller/ControllerTrait.php:282
...sf4/src/Controller/DefaultController.php:15
...sf4/vendor/symfony/http-kernel/HttpKernel.php:151
...sf4/vendor/symfony/http-kernel/HttpKernel.php:66
...sf4/vendor/symfony/http-kernel/Kernel.php:190
...sf4/vendor/symfony/http-kernel/Client.php:68
...sf4/vendor/symfony/framework-bundle/Client.php:131
...sf4/vendor/symfony/browser-kit/Client.php:312
...sf4/tests/DefaultControllerTest.php:12

If I return a response from the controller:

return new Response('Not found', Response::HTTP_NOT_FOUND)

then the tests pass.

I have create a minimal example here.

@dmaicher
Copy link
Contributor

dmaicher commented Dec 4, 2017

composer req twig solves it 😉

There is no default ExceptionListener otherwise that will handle the exception and for example render an error page (like the default twig listener does).

Wondering if there should be some lightweight alternative by default?

@gjuric
Copy link
Contributor Author

gjuric commented Dec 4, 2017

Yes, it looks like Twig solves a part of it.

After adding Twig the tests pass, but I get an error in the output:

#!/usr/bin/env php
PHPUnit 6.5.2 by Sebastian Bergmann and contributors.

Testing Project Test Suite
2017-12-05T00:48:45+01:00 [error] Uncaught PHP Exception Symfony\Component\HttpKernel\Exception\NotFoundHttpException: "Not Found" at .../vendor/symfony/framework-bundle/Controller/ControllerTrait.php line 282
..                                                                  2 / 2 (100%)

Time: 1.61 seconds, Memory: 20.00MB

OK (2 tests, 2 assertions)

edit: Also, I am not sure why am I seeing the shebang line in the output.

@sroze
Copy link
Contributor

sroze commented Dec 5, 2017

Shouldn't we add one in the FrameworkBundle? 🤔 @nicolas-grekas @javiereguiluz

@gjuric
Copy link
Contributor Author

gjuric commented Dec 5, 2017

After thinking about it, the behavior actually makes sense.

If there is no templating engine you can not define error pages to be displayed so the FW doesn't know what to return in that case.

But the issue of showing the error in the output of phpunit after adding templating still remains.

@dmaicher
Copy link
Contributor

dmaicher commented Dec 5, 2017

If there is no templating engine you can not define error pages to be displayed so the FW doesn't know what to return in that case.

But there could maybe be a default listener that returns a response with the correct status code and a body (depending on _format maybe?) that contains the status message like Not Found. Without any templating. Just an idea 😄

But the issue of showing the error in the output of phpunit after adding templating still remains.

That seems to be logger output?

@gjuric
Copy link
Contributor Author

gjuric commented Dec 5, 2017

I agree on the response with the default status code. The body should probably be something like this:

<h1><?php echo $responseCode.' '.\Symfony\Component\HttpFoundation\Response::$statusTexts[$responseCode];?></h1>

with the correct response code in the headers.

Regarding the logger output, I tried changing the framework.php_errors.log to false but it didn't help. After requiring the monolog bridge the output doesn't show up anymore.

    php_errors:
        log: false

@xabbuh
Copy link
Member

xabbuh commented Dec 5, 2017

You can silence the output during tests by setting the SHELL_VERBOSITY env var to -1 (for example, in your phpunit.xml.dist file like done here: https://github.com/symfony/recipes/blob/master/symfony/phpunit-bridge/3.3/phpunit.xml.dist#L16).

@gjuric
Copy link
Contributor Author

gjuric commented Dec 5, 2017

Yes, I did set it to -1 (actually, it was set like that by default), and the output is still there https://github.com/gjuric/sf4_phpunit/blob/1697fc6e7b5ea8838dd6d79eb2782f0abda4fa03/phpunit.xml.dist#L16

I also tried changing that number but I didn't find what are the available options and there meanings for the SHELL VERBOSITY variable.

@zytzagoo
Copy link

zytzagoo commented Dec 5, 2017

Setting it to -1 still prints errors (and above) if I'm reading https://github.com/symfony/symfony/blob/4.0/src/Symfony/Component/HttpKernel/Log/Logger.php correctly (if that's the logger being used, grepped quickly for SHELL_VERBOSITY) -- so that part seems to be working as expected?

@xabbuh
Copy link
Member

xabbuh commented Dec 5, 2017

@gjuric @zytzagoo Yes, errors will still be printed. Looks like I read that too fast.

@jvahldick
Copy link
Contributor

Hi guys, do you have some update here?

I have the issue here, and for some reason when I try to run the tests isolating the processes I get it as an invalid test.

php bin/phpunit --process-isolation

@rodrigodiez
Copy link

Seems same as #25844

@junowilderness
Copy link
Contributor

There are some ideas in #25844 for solving this in all cases, similar to what @dmaicher mentioned above.

@junowilderness
Copy link
Contributor

I am suggesting we close this in favor of #25844 with PR #26138

@dmaicher
Copy link
Contributor

@nicolas-grekas this was fixed by #26138 ? 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants