Test Ordering and Random Execution

[RobDWaller]

Hi, I was just taking a look at the unit tests for Slim 4 and noticed if you set them to run in a random order you generate errors due to state inconsistencies.

Are the tests setup to execute in a defined order?

If you can enable / support random ordering you can implement tools like Infection PHP which will further enhance the improvements made in Slim 4.

These are the settings I added to PHPUnit config:

executionOrder="random"
resolveDependencies="true"

This is some of the test output generated:

I ask this mainly out of curiosity, I accept there are perfectly legitimate reasons for defined test ordering.

[JustSteveKing]

I think one of the things with testing at the framework level, is that randomising the tests is not an accurate depiction of how the framework should work and be tested. There will be the base layer tests which is testing base functionality at a unity level, but after that testing through the application layer needs to be done in a specific way.

[RobDWaller]

I sort of agree, but the 'randomising' issue relates mainly to test encapsulation. It may be encapsulation represents too much work and not enough value in this case.

Happy for this to be closed if it isn't seen as an issue, I just raised it because I noticed it when I was reacquainting myself with the framework.

[l0gicgate]

@RobDWaller we should probably fix that. Looking back it’s really a bit of laziness while writing tests to not resetup everything properly on each test that leads to this.

[RobDWaller]

@l0gicgate I'm sure it wasn't laziness, this sort of thing takes a great deal of work and it's difficult to get it all right immediately. I'd be happy to have a look and try to fix some of these issues.

[l0gicgate]

@RobDWaller all help is welcome! Feel free to raise a PR if you have time to do it! Thank you for reporting this.

[jdrieghe]

I've been looking into this and the main thing holding back the test randomization is the global state in static member variables of factory classes here: https://github.com/slimphp/Slim/tree/4.x/Slim/Factory.

If we want to keep this architectural choice around, I think we need to find a clean way to rebuild a fresh application with cleared global state in the tests. I don't mind spending some time on this if you believe this is a good solution.

[pawel-slowik]

Hi, I'd like to help with this issue.

Currently there are three tests that fail when run in isolation:

./vendor/bin/phpunit --filter 'Slim\\Tests\\RouteTest::testControllerMethodAsStringResolvesWithContainer'

./vendor/bin/phpunit --filter 'Slim\\Tests\\RouteTest::testChangingCallableWithNoContainer'

./vendor/bin/phpunit --filter 'Slim\\Tests\\Factory\\ServerRequestCreatorFactoryTest::testSetServerRequestCreator'

For the first two I have bisected to find the commits that broke them and I think I know how to fix them. I'll submit PRs shortly.

The last test (ServerRequestCreatorFactory related) is broken since the beginning. I have managed to "sort of fix" it by adding:

ServerRequestCreatorFactory::setSlimHttpDecoratorsAutomaticDetection(false);

but that's a "solution" based entirely on guesswork (basically disabling / enabling other tests until this one passes).
I think that the factory tests need a way to rebuild the application with cleared global state, just as @jdrieghe pointed out.

[pawel-slowik]

Also, it's possible that some tests will still fail when randomized even though they are fixed to run correctly in isolation. But I think getting them to run in isolation is a good first step.

[l0gicgate]

@pawel-slowik thank you for helping with this.

[pawel-slowik]

As for the last failing test (testSetServerRequestCreator), I think it is inconsistent with the tested code.

As I understand it, the test says: after setServerRequestCreator($x), create() must return a request object created by $x.

But the code says: after setServerRequestCreator($x), create() will return either a request object created by $x or another object that decorates it.

That's why this test only succeeds when run after another test that disables decorator detection with:

ServerRequestCreatorFactory::setSlimHttpDecoratorsAutomaticDetection(false);

I'm not sure how to proceed further because I don't know which one is the expected / desired behavior.

Personally, in this case I find the automatic decorators surprising. When I explicitly say "use my FooFactory for creating Foo objects", I'm expecting Foo objects in return, not something that wraps / decorates them. But I've never actually used setServerRequestCreator, so I could be missing a use case here.

[pawel-slowik]

Here's a PR that fixes the last test to run correctly in isolation: #3017

As for fully randomizing the tests: this is harder than I thought it'd be due to static properties. For example, if a test executes:

ServerRequestCreatorFactory::setServerRequestCreator($serverRequestCreatorProphecy->reveal());

then later the test

Slim\Tests\Factory\ServerRequestCreatorFactoryTest::testCreateAppWithAllImplementations

will fail, because it relies on ServerRequestCreatorFactory::$serverRequestCreator not being set. And there's no way to unset $serverRequestCreator once it has been set.

Similarily, an earlier invocation of

Psr17FactoryProvider::setFactories([]); 

will cause

Slim\Tests\AppTest::testRunWithoutPassingInServerRequest

to fail, because it relies on a factory being available from Psr17FactoryProvider.

What can we do about it?

1. Add the @backupStaticAttributes annotation where necessary. This makes the tests run a lot slower (12 seconds instead of 1 s for two tests) and also crashes PHPUnit 😞

2. Add the @runInSeparateProcess annotation where necessary. This works, but makes the tests run a lot slower (37 seconds instead of 5 s for the entire testsuite).

3. Reset the problematic static properties to known values between tests. This feels somewhat dirty, but we're doing that already:

   Slim/tests/Factory/Psr17/SlimHttpServerRequestCreatorTest.php

   Lines 23 to 39 in 1b098e7

   	/**
   	* We need to reset the static property of SlimHttpServerRequestCreator back to its original value
   	* Otherwise other tests will fail
   	*/
   	public function tearDown(): void
   	{
   	$serverRequestCreatorProphecy = $this->prophesize(ServerRequestCreatorInterface::class);
   	$slimHttpServerRequestCreator = new SlimHttpServerRequestCreator($serverRequestCreatorProphecy->reveal());
   	$serverRequestDecoratorClassProperty = new ReflectionProperty(
   	SlimHttpServerRequestCreator::class,
   	'serverRequestDecoratorClass'
   	);
   	$serverRequestDecoratorClassProperty->setAccessible(true);
   	$serverRequestDecoratorClassProperty->setValue($slimHttpServerRequestCreator, ServerRequest::class);
   	}

4. Do 3., but in a more well thought out / clean way. Basically what @jdrieghe said:

   we need to find a clean way to rebuild a fresh application with cleared global state in the tests

   The problem with that is we can't read the default / original values of static properties, because ReflectionClass::getDefaultProperties doesn't work for static properties of user defined classes:
   https://www.php.net/manual/en/reflectionclass.getdefaultproperties.php
   I can't think of any other clean way of handling this.

5. Statics are evil. Drop them in Slim 5.0, close the issue with "wontfix until 5.0". 😛

Thoughts, anyone?

[l0gicgate]

Add the @runInSeparateProcess annotation where necessary. This works, but makes the tests run a lot slower (37 seconds instead of 5 s for the entire testsuite).

@pawel-slowik I think this is the best option. I don't really care that tests take longer to run.

[pawel-slowik]

@l0gicgate note that after randomizing the test execution order we'll sometimes see messages emitted by tests/Handlers/ErrorHandlerTest.php. Previously these were not visible because of this:

Slim/tests/AppTest.php

Lines 57 to 60 in 9f6a54f

	public static function setupBeforeClass(): void
	{
	ini_set('error_log', tempnam(sys_get_temp_dir(), 'slim'));
	}

The messages have no effect on test status.

Other than that, I think we're all set.

[l0gicgate]

The messages have no effect on test status.

@pawel-slowik great! can I close this issue?

[pawel-slowik]

@l0gicgate yes, it's OK to close it.