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

Test Suite Proposed Changes #2585

Closed
3 tasks done
l0gicgate opened this issue Feb 19, 2019 · 6 comments
Closed
3 tasks done

Test Suite Proposed Changes #2585

l0gicgate opened this issue Feb 19, 2019 · 6 comments
Assignees
Milestone

Comments

@l0gicgate
Copy link
Member

l0gicgate commented Feb 19, 2019

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

  • Remove Pimple
  • Upgrade to PHPUnit 7.5
  • Add Code Coverage directory to .gitignore
@l0gicgate l0gicgate added this to the 4.0 milestone Feb 19, 2019
@l0gicgate l0gicgate self-assigned this Feb 19, 2019
@akrabat
Copy link
Member

akrabat commented Feb 19, 2019

Pimple should be mocked out for 4.x testing.

@akrabat
Copy link
Member

akrabat commented Feb 19, 2019

Agreed re PHPUnit upgrade.

@l0gicgate
Copy link
Member Author

l0gicgate commented Feb 19, 2019

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
Copy link
Contributor

bnf commented Mar 3, 2019

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): e53dc95
  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
Copy link
Member

akrabat commented Mar 3, 2019

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
Copy link
Member Author

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 😂

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

No branches or pull requests

3 participants