zenstruck / foundry

A model factory library for creating expressive, auto-completable, on-demand dev/test fixtures with Symfony and Doctrine.
https://symfony.com/bundles/ZenstruckFoundryBundle/current/index.html
MIT License
643 stars 70 forks source link

Foundry should not remove symfony-s errorHandlers #640

Closed mmarton closed 3 months ago

mmarton commented 3 months ago

Hi!

starting from 1.38 foundry removes symfonys error handlers in KernelHelper::shutdownKernel(). You should not remove something that's another library started. Or at least not in a forced way, just include the helper and let the user decide if he want to call it.

There is a proposed way in the issue mentioned in the Kernelhelper class to handle this. https://github.com/symfony/symfony/issues/53812#issuecomment-1962740145

Using it with Foundry displays 'Test code or tested code removed error handlers other than its own' warning.

my test


use App\Security\UserRole;
use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
use Symfony\Component\HttpFoundation\Response;
use Tests\Factory\UserFactory;
use Zenstruck\Foundry\Test\Factories;
use Zenstruck\Foundry\Test\ResetDatabase;

final class ControllerTest extends WebTestCase
{
    use Factories;
    use ResetDatabase;

    private static string $baseUrl = '';

    public static function setUpBeforeClass(): void
    {
        self::$baseUrl = 'http://'.self::getContainer()->getParameter('admin_host');
        self::ensureKernelShutdown();
    }

    public function testController1(): void
    {
        $client = self::createClient();
        $client->request('GET', self::$baseUrl.'/some-admin-url');

        self::assertResponseStatusCodeSame(Response::HTTP_FORBIDDEN);
    }

    public function testController2(): void
    {
        $user = UserFactory::createOne([
            'email' => 'test@email.com',
            'roles' => [UserRole::ADMIN->value],
        ]);

        self::ensureKernelShutdown();

        $client = $this->createLoggedInClient($user->_real());
        $client->request('GET', self::$baseUrl.'/some-admin-url');

        self::assertResponseStatusCodeSame(Response::OK);
}

Running this with your solution:

Test code or tested code removed error handlers other than its own

Adding set_exception_handler([new ErrorHandler(), 'handleException']); to tests/bootstrap.php:

Test code or tested code removed error handlers other than its own

Adding set_exception_handler([new ErrorHandler(), 'handleException']); to tests/bootstrap.php AND commenting out the call to KernelHelper:

no warning, no risky test, everything works fine

nikophil commented 3 months ago

Hi @mmarton

this is a complex topic, the main problem with the workardound set_exception_handler([new ErrorHandler(), 'handleException']); added in boostrap.php is that Symfony exception handler does not let deprecation warnings bubble up to PHPUnit's one...

Still, I do understand that the chosen solution is unfortunate as well. more over the problem is between Symfony and PHPUnit, and Foundry has nothing to deal with it.. But for the migration between foundry v1 and v2, the ability to display deprecation is kinda super helpful.

The problem occurs because methods marked as #[BeforeClass] in the traits occur before TestCase::setUpBeforeClass() and then Symfony's exception handler is not removed afterwards if the kernel is booted there.

I think our hands are tied here :disappointed:

(I've been thinking about introducing a PHPUnit Extension for Foundry for a long time, maybe that would be a way to mitigate this problem. But we would still need to mess around with Symfony's exception handler, which is not Foundry's responsibility, I admit)

@kbond any thoughts?

kbond commented 3 months ago

@mmarton, to confirm, are you using phpunit 10+?

mmarton commented 3 months ago

@mmarton, to confirm, are you using phpunit 10+?

Yes, the latest 11.2 version

nikophil commented 3 months ago

after a lot a of digging, I've just figured out that the main reason of why we've introduced this whole "error handlers resetting" stuff is because when the kernel is booted in a "before class" hook, the deprecation won't bubble up in PHPUnit (because Symfony registers its error handler before PHPUnit, and then PHPUnit won't register its own error handler if one already exists. see both here and here

So we had to find a way to prevent this when using ResetDatabase. But it is not needed to reset the error handler in a setup() hook (aka #[Before]), and it is where the problem Test code or tested code removed error handlers other than its own comes from. At then end, the solution would be to not only reset error handlers in ResetDatabase::_resetDatabase() which is the only place where Foundry boots the kernel in a "before class" hook.

nikophil commented 3 months ago

I've released v1.38.2 & v2.0.3. I'm closing the issue, feel free to reopen it if you still have problems with this

mmarton commented 3 months ago

Just tested with 1.38.2 and it fixed my issue. Thanks