Add support for PHPUnit 6.0 #26

johnkary · 2017-02-13T18:02:25Z

phpunit-speedtrap master branch now supports a future 2.0 of SpeedTrapListener. This PR updates the listener to only support PHPUnit 6.0 and increases the minimum PHP version to 7.0.

This PR also adds more unit tests I had been using locally during development to ensure the listener functioned correctly. Long-term I would like a more robust test strategy that does not rely on visually examining the PHPUnit test runner output.

Should any other changes be made before merge? Please add your reaction to this post or comment below. Thanks! 🚀

This library never officially supported HHVM. However, HHVM support was removed from PHPUnit in 4.8.28 via sebastianbergmann/phpunit@bde83ef

Library maintainers may use phpunit.xml for local test configuration

TomasVotruba · 2017-02-13T20:06:25Z

I added few comments about code that is different to #25

Why not just use that PR? I miss some feedback :(

TomasVotruba · 2017-02-13T20:07:45Z

Tests are not passing either: https://travis-ci.org/johnkary/phpunit-speedtrap/jobs/201215016#L239

TomasVotruba · 2017-02-13T20:08:02Z

src/JohnKary/PHPUnit/Listener/Tests/ExceptionalTest.php

+    public function testExceptionCanBeThrownInTest()
+    {
+        $this->expectException('InvalidArgumentException');
+        $this->expectExceptionMessage('CODE1');


This is missing in PHPUnit 6.0

Have you tested the code locally?

expectExceptionMessage() is a valid method in PHPUnit 6.0: https://github.com/sebastianbergmann/phpunit/blob/6.0/src/Framework/TestCase.php#L588

TomasVotruba · 2017-02-13T20:08:43Z

src/JohnKary/PHPUnit/Listener/Tests/SomeSlowTest.php

@@ -0,0 +1,86 @@
+<?php


strict types are missing

Strict Types are not required when using PHP 7.

Why would you not use it?

This library will absolutely transition to supporting PHP 7 features like strict types :) In a separate PR. If we add so many changes into a PR it becomes more time consuming to review. I prefer keeping a single PR focused on one feature at a time.

The feature for #26, this PR, is to support PHPUnit 6.0. So before coding I asked myself, "What is the least amount of work that could be done to support PHPUnit 6.0?" This PR does that without adding other features. And if we wanted to do nothing else, this PR could be merged and 2.0 released to bare minimum support PHPUnit 6 and PHP 7.

Supporting these new versions absolutely opens the door to other great improvements. But those improvements are secondary to simply adding support first.

Does that make more sense?

It makes sense.

I still don't understand why did you create a new PR for work I've already done didn't gave me any feedback.

I feel ignored at the moment.

@TomasVotruba I'm sorry! I don't want you to feel ignored. I really appreciate your work on #25 and value your input on this project.

Your PR has some changes I would love to incorporate, but it currently makes too many changes within one PR. It adds PHPUnit 6 and PHP 7 support, but also makes some changes to the core structure of the library that aren't in the project's best interest.

After this PR #26 is merged let's work together to bring in some changes you propose like PHP 7 strict types, scalar type hints and PSR-4 compatibility.

Your PR has some changes I would love to incorporate, but it currently makes too many changes within one PR. It adds PHPUnit 6 and PHP 7 support, but also makes some changes to the core structure of the library that aren't in the project's best interest.

Why not just tell me there? I didn't know. And I can't change the code without your feedback.

TomasVotruba · 2017-02-13T20:09:01Z

src/JohnKary/PHPUnit/Listener/Tests/SomeSlowTest.php

+    /**
+     * @param int $ms Number of additional microseconds to execute code
+     */
+    private function extendTime($ms)


scalar type is missing

TomasVotruba · 2017-02-13T20:09:09Z

.gitignore

@@ -1,2 +1,3 @@
+/phpunit.xml


In this repository PHPUnit will use phpunit.xml.example when executed. If a developer wishes to further configure PHPUnit in their dev environment when running phpunit-speedtrap tests they copy phpunit.xml.example to phpunit.xml and customize. PHPUnit then uses phpunit.xml instead of phpunit.xml.example

Ignoring phpunit.xml will prevent git from trying to add the developer's local customizations.

This is common practice among PHP libraries. For example Symfony:

Ignores phpunit.xml

Provides phpunit.xml.dist

How does you phpunit.xml differ?

TomasVotruba · 2017-02-13T20:09:28Z

.travis.yml

-  - PHPUNIT=5.5.*
-  - PHPUNIT=5.6.*
-  - PHPUNIT=5.7.*
+  - PHPUNIT=6.0.*


This duplicates composer.json, no need for it.

Once PHPUnit 6.1 is released we'll need a strategy for Travis to run tests against PHPUnit 6.0 and 6.1. Entries in .travis.yml do not affect versions installed by projects that require phpunit-speedtrap using Composer.

How do you ensure this package will be used with correct PHPUnit?

The version constraints in phpunit-speedtrap composer.json define this library's version compatibility. A developer installing it will have a mix of different PHP versions and their project's version constraints. Composer figures out which version to install.

.travis.yml allows us to install each major PHPUnit version. It's like each Travis run creates a separate project that requires phpunit-speedtrap and a different PHPUnit version. So a test run will use phpunit-speedtrap plus the latest PHPUnit minor version we support. PHPUNIT=6.0.* will install PHPUnit 6.0.6 and run the phpunit-speedtrap test suite.

We could also add testing 6.1-dev.

TomasVotruba · 2017-02-13T20:10:06Z

composer.json

-        "php": ">=5.6",
-        "phpunit/phpunit": ">=4.7"
+        "php": ">=7.0",
+        "phpunit/phpunit": ">=6.0"


Allowing higher major version is dangerous. This code is not compatible with PHPUnit 7.0, when released.

I tend to favor optimistic version constraints, but I'm OK dialing it back to support >=6.0 and < 7.0 using ^6.0. Fixed in a2c2bb0.

https://semver.mwl.be/#?package=phpunit%2Fphpunit&version=%5E6.0&minimum-stability=stable

TomasVotruba · 2017-02-13T20:10:32Z

composer.json

-        "php": ">=5.6",
-        "phpunit/phpunit": ">=4.7"
+        "php": ">=7.0",
+        "phpunit/phpunit": ">=6.0"
    },
    "autoload": {
        "psr-0": {


Prefer PSR-4. PSR-0 is deprecated over 2,5 years now: http://www.php-fig.org/psr/psr-0/

Agreed. However that change can be handled in a separate PR.

TomasVotruba · 2017-02-13T20:10:39Z

phpunit.xml.example

@@ -17,9 +17,6 @@
        <testsuite name="Project Test Suite">
            <directory>src/*/*/*/Tests</directory>
        </testsuite>
-        <testsuite name="tests">
-            <directory>tests</directory>
-        </testsuite>


TomasVotruba · 2017-02-13T20:10:59Z

src/JohnKary/PHPUnit/Listener/SpeedTrapListener.php

     * @param int                         $time Test execution time in milliseconds
     */
-    protected function addSlowTest(\PHPUnit_Framework_TestCase $test, $time)
+    protected function addSlowTest(TestCase $test, $time)


Why protected? Same for all protected methods.

Missing return types. Same for most own methods bellow.

addSlowTest does not return a value.

Protected methods allow developers to customize how they wish to determine their own tests slow. This library will preserve extensibility allowing developers to override methods.

I see. Why not prefer composer over inheritance?

http://ocramius.github.io/blog/when-to-declare-classes-final/

I'm normally in favor of using composition instead of inheritance. This project doesn't go that route because frankly there's not much interesting logic here :)

We could decouple the logic that determines slowness from the PHPUnit Listener. We'd end up with two classes: A) The class determining slowness; B) The PHPUnit Listener that is uses Class A.

That would open us to unit testing the code determining slowness in isolation. But that logic is very simple and not particularly hard to debug. It's much more interesting to see an integration test with how PHPUnit responds when wired with this Listener.

If we had two classes, anyone wanting to customize the library's behavior would need to create their own class then inject our own instance. Often developers just want to make a quick subclass and override where necessary.

If this library had more private details with more complexity perhaps I'd consider making its implementation final. However this library is so small it just isn't worth the effort to work against the norm.

So far nobody has asked for customizations or features that cannot be handled by overriding one of the existing extension points.

Ocarmius, whom I've followed and interacted with for a long time, is a lead and maintainer of various Doctrine projects which have enormous complexity. Their libraries have more vested interest in defining exactly how their project will work, without wanting to support all the crazy things developers want to do (mostly out of negligence) with their domain models.

Final might make sense for them. It does not for this project.

TomasVotruba · 2017-02-13T20:12:11Z

src/JohnKary/PHPUnit/Listener/Tests/SomeSlowTest.php

+    }
+    public function provideTime()
+    {
+        return array(


When reading this, I assume it works on PHP 5.3.

array() is compatible syntax for PHP 7. It may not be preferred, but this PR does not attempt to add PHP 7 enhancements. It strictly focuses on PHPUnit 6.0 compatibility.

Why would you bump to PHPUnit that requires PHP 7 and not use PHP 7 features?

TomasVotruba · 2017-02-20T15:32:57Z

👍 Great job!

johnkary added 6 commits February 13, 2017 10:04

Require PHP 7.0+

6f39096

Require PHPUnit 6.0+ and add PHPUnit 6 support to Listener

9ea93d9

HHVM no longer supported

ec2a2b5

This library never officially supported HHVM. However, HHVM support was removed from PHPUnit in 4.8.28 via sebastianbergmann/phpunit@bde83ef

Remove old tests

fccd648

Add tests that verify Listener works as intended

a21e642

Ignore phpunit.xml

bc13e3f

Library maintainers may use phpunit.xml for local test configuration

TomasVotruba reviewed Feb 13, 2017

View reviewed changes

johnkary added 4 commits February 16, 2017 11:57

Assume SpeedTrapListener compatible with PHPUnit >=6.0, <7.0

a2c2bb0

Exclude tests from test coverage report

d198b03

Travis runs all testsuites

b2be7e3

Travis already updates Composer prior to before_script running

f94d09e

johnkary merged commit 6126919 into master Feb 20, 2017

johnkary mentioned this pull request Feb 20, 2017

phpunit6 namespaced class support #21

Closed

johnkary deleted the support60 branch March 12, 2017 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for PHPUnit 6.0 #26

Add support for PHPUnit 6.0 #26

johnkary commented Feb 13, 2017

TomasVotruba commented Feb 13, 2017 •

edited

Loading

TomasVotruba commented Feb 13, 2017

TomasVotruba Feb 13, 2017

johnkary Feb 16, 2017

TomasVotruba Feb 13, 2017

johnkary Feb 16, 2017

TomasVotruba Feb 17, 2017

johnkary Feb 17, 2017

TomasVotruba Feb 17, 2017 •

edited

Loading

johnkary Feb 17, 2017

TomasVotruba Feb 17, 2017

TomasVotruba Feb 13, 2017

TomasVotruba Feb 13, 2017

johnkary Feb 16, 2017

TomasVotruba Feb 17, 2017

TomasVotruba Feb 13, 2017

johnkary Feb 16, 2017

TomasVotruba Feb 17, 2017

johnkary Feb 17, 2017

TomasVotruba Feb 13, 2017

johnkary Feb 16, 2017

TomasVotruba Feb 13, 2017

johnkary Feb 16, 2017

TomasVotruba Feb 13, 2017

TomasVotruba Feb 13, 2017

TomasVotruba Feb 13, 2017

johnkary Feb 16, 2017

TomasVotruba Feb 17, 2017

johnkary Feb 17, 2017

TomasVotruba Feb 13, 2017

johnkary Feb 16, 2017

TomasVotruba Feb 17, 2017

TomasVotruba commented Feb 20, 2017

Add support for PHPUnit 6.0 #26

Add support for PHPUnit 6.0 #26

Conversation

johnkary commented Feb 13, 2017

TomasVotruba commented Feb 13, 2017 • edited Loading

TomasVotruba commented Feb 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasVotruba Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomasVotruba commented Feb 20, 2017

TomasVotruba commented Feb 13, 2017 •

edited

Loading

TomasVotruba Feb 17, 2017 •

edited

Loading