Slim: Test Suite Proposed Changes

[l0gicgate]

We need to tidy up our test suite for Slim 4. I would like to propose the following:

Add pre-defined code coverage directory in phpunit.xml.dist
We should add coverage-html generation in our phpunit configuration and also add that pre-defined directory to our .gitignore

<phpunit>
    ...
    <logging>
        <log
            type="coverage-html"
            target="./coverage"
            lowUpperBound="20"
            highLowerBound="50"
        />
    </logging>
</phpunit>

Get rid of Pimple as a Dev Dependency
All we need for our test suite is a simple implementation of ContainerInterface, we don't to anything but simple key/value setting. Less dependencies to install means faster builds. Plus the Pimple instantiation/synthax is terrible:

use Pimple\Container as Pimple;
use Pimple\Psr11\Container as Psr11Container;

public function testRequiringContainer() {
    $pimple = new Pimple();
    $pimple['key'] = 'value';
    $container = new Psr11Container($pimple);
    ...
}

We need to update PHPUnit to 7.5 and remove deprecated methods
Currently we are using TestCase::assertAttributeEquals a lot which is a deprecated method. Will need to refactor all tests using deprecated methods from this list

Break up AppTest into smaller modules
This may take some time, but I want to make App a little bit more modular in the near future which will hopefully reduce the size of the AppTest. A lot of the test cases are from proxying Router methods.

Status

• [x] Remove Pimple
• [x] Upgrade to PHPUnit 7.5
• [x] Add Code Coverage directory to .gitignore

[akrabat]

Pimple should be mocked out for 4.x testing.

[akrabat]

Agreed re PHPUnit upgrade.

[l0gicgate]

Okay and what about the code coverage bit @akrabat? This is more so just for developer user experience, it's annoying when I start a new branch I have to update .gitignore every time, I do generate coverage locally to make sure I've done all the required tests.

At the minimum we should at least add a predefined coverage-html output directory to .gitignore

[bnf]

I like the idea to remove the pimple dev dependency by mocking ContainerInterface in unit tests.
I personally would prefer mock generators like the phpunit MockBuilder or Prophecy over implementing separate Mock* classes, as mock classes basically become yet another implementation, and mock generators allow assertions (without static call counters).

Now, when comparing MockBuilder and Prophecy mocks, I'd propose to go with Prophecy for four reasons:

1. Prophecy is stricter than MockBuilder. It does not allow to create mixed proxy/stub/mock classes. That means: No hidden (proxy) calls to real functions of the mocked class, which happens when one method was not listed in the MockBuilder's setMethod but is called from the tested code. (Note: This problem could be "solved" by permitting $this->createMock() only, and not $this->getMockBuilder(…))
2. It may be easier to understand: There is one object (ObjectProphecy) and a separate mocked instance (as generated by ObjectProphecy::reveal). There is no mixture in types, as in MockBuilder.
3. I think it is more flexible. It allows to define promises that mutate state – which is much easier to write than other approaches do (call order prediction) and describes the dependency in a very formal way (of course that's opinionated). I think that's better than predicting the order of calls, like usually done with phpunit MockBuilder. Please see the following commit, how this can be used to mock the behaviour of withAddedHeader for PSR-7. I think that would allow to remove the specific PSR-7 implementation from unit tests as well (if we want that): https://github.com/slimphp/Slim/commit/e53dc953582a980ef4bff7be6e5c2b7ac4e2279d
4. Sebastian Bergmann wrote "If I were to create a new mocking framework today it would probably look a lot like Prophecy. Which is why PHPUnit 4.5 introduced out-of-the-box support for it." https://thephp.cc/news/2015/02/phpunit-4-5-and-prophecy and also "I'll probably recommend Prophecy over PHPUnit's own implementation at some point, yes." https://twitter.com/s_bergmann/status/536440940102438912

I've prepared a pull request in #2602 to demonstrate how that change would look like when using Prophecy.

[akrabat]

I have more exprience with createMock, so that tends to be my go-to. However, Prophecy is fine with me if we have people willing to (a) write it and (b) help the newbies!

[l0gicgate]

I'm definitely in favor of using Prophecy instead of PHP Unit Mocks. Thanks to @bnf for turning me onto it in #2591 😄

@akrabat looks like you have some learning to do 😂