Skip to content
This repository has been archived by the owner on Jan 15, 2024. It is now read-only.

Upgrade to phpunit 6.4 #14

Closed
wants to merge 3 commits into from
Closed

Conversation

foaly-nr1
Copy link

PHPUnit 5.7 is the old stable release series.
It became stable on December 2, 2016.
Support for PHPUnit 5 ends on February 2, 2018.
https://phpunit.de/

@coveralls
Copy link

Coverage Status

Coverage increased (+2.2%) to 92.409% when pulling 3953436 on foaly-nr1:phpunit-6 into 34732e5 on kleijnweb:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+2.2%) to 92.409% when pulling fc1e221 on foaly-nr1:phpunit-6 into 34732e5 on kleijnweb:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+2.3%) to 92.555% when pulling b1d3570 on foaly-nr1:phpunit-6 into 34732e5 on kleijnweb:master.

@foaly-nr1
Copy link
Author

Build #35 executed 2 tests less than build #32.

This is because of two data providers that were incorrectly annotated as tests:

@coveralls
Copy link

Coverage Status

Coverage increased (+2.3%) to 92.555% when pulling 732e120 on foaly-nr1:phpunit-6 into 34732e5 on kleijnweb:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+2.3%) to 92.555% when pulling 732e120 on foaly-nr1:phpunit-6 into 34732e5 on kleijnweb:master.

@kleijnweb
Copy link
Owner

Thank you for the MR. I am not a fan of the style changes you made (although there is something to be said for invoking static methods, statically) so I applied the other changes manually to master. Thanks again.

@foaly-nr1
Copy link
Author

foaly-nr1 commented Nov 22, 2017

Thanks for your explanation! Below my response 😊

The interface changed in 6.4 – the methods used to be instance methods, now they’re static. So I changed the method calls. PHP isn’t strict enough to enforce the interface, but to me it’s still a breach of contract. Not just code style.

@kleijnweb
Copy link
Owner

Nope. The methods have been static for over 10 years. Don't take my word for it, check out this 11 year old class: https://github.com/sebastianbergmann/phpunit/blob/2.3/Framework/Assert.php

Over the course of those 10+ years, most people including myself have been using the instance invocation. The examples in PHPUnit use it (which is probably the main reason why everyone is doing it).

In other, non-PHPUnit code, I would not do this (breach of contract is a bit strong, but at the very least it is obfuscation). But for PHPUnit tests I don't see much harm and thus little reason to break with convention.

As far as the "test" method name prefix vs @test annotation, I simply prefer the method names read like BDD scenarios. The test prefix makes that a little awkward when read aloud. Also some features, specifically data providers have no other practical way of configuring, so what the hell, might as well use annotations to mark tests too.

None of this is a specifically hot button issue for me, I would much rather discuss design and features, but I hope this sheds some light on why didn't merge your PRs.

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

Successfully merging this pull request may close these issues.

None yet

3 participants