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

Add support for PHPUnit 6 using class_alias in shim.php #4142

Merged
merged 21 commits into from Apr 25, 2017

Conversation

MontealegreLuis
Copy link
Contributor

@MontealegreLuis MontealegreLuis commented Apr 18, 2017

I added support for PHPUnit 6 using class_alias as discussed in #3995

  • Tests are passing in Travis https://travis-ci.org/MontealegreLuis/Codeception/builds/223307775
  • I had to include PHPUnit_Util_Log_JSON, PHPUnit_Util_Log_TAP and PHP_Util_String in phpunit5-loggers.php given they were removed in PHPUnit 6.
  • There's a new method in the PHPUnit listener addWarning, so there are now 2 class definitions in Codeception\PHPUnit\Listener, one for PHPUnit 5 and another for 6, which are loaded conditionally.
  • Method setExpectedException was removed in PHPUnit 6 from PHPUnit\Framework\TestCase so I added it back to the classes that inherit from TestCase. It calls the parent method if it exists, otherwise it will call the new methods expectException, expectExceptionMessage, and expectExceptionCode.
  • UnitCept is checking for an exception class name so there's one more if statement there.

Thanks in advance!

* Add an empty implementation for the
  method addWarning, added to the
  TestListener interface in PHPUnit6
* Load this implementation only if
  PHPUnit 6 is being used, otherwise use
  the current implementation
* For PHPUnit 5 forward the call to the parent class.
* For PHPUnit 6 use expectException, expectExceptionMessage, expectExceptionCode.
* Both the JSON and TAP loggers were removed in PHPUnit 6.
Added method to the following classes

* MysqlTest, PostgresTest, MongoDbTest and RedisTest
When using PHP 5.6 class_exists returns true for 'PHPUnit\Framework\TestCase'.
PHUnit 6 can only be installed on PHP 7, now it's part of the main condition.
/**
* String helpers.
*/
class PHPUnit_Util_String

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Each class must be in a namespace of at least one level (a top-level vendor name)
  • Class name PHPUnit_Util_String is not in camel caps format

/**
* A TestListener that generates JSON messages.
*/
class PHPUnit_Util_Log_JSON extends PHPUnit_Util_Printer implements PHPUnit_Framework_TestListener

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Each class must be in a file by itself
  • Each class must be in a namespace of at least one level (a top-level vendor name)
  • Class name PHPUnit_Util_Log_JSON is not in camel caps format

* A TestListener that generates a logfile of the
* test execution using the Test Anything Protocol (TAP).
*/
class PHPUnit_Util_Log_TAP extends PHPUnit_Util_Printer implements PHPUnit_Framework_TestListener

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Each class must be in a file by itself
  • Each class must be in a namespace of at least one level (a top-level vendor name)
  • Class name PHPUnit_Util_Log_TAP is not in camel caps format

}
} else {
// Use PHPUnit 5 TestListener definition
class Listener implements \PHPUnit_Framework_TestListener

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each class must be in a file by itself

* Added missing replacements.
* testFailedSeeInPopup was using annotations that were conflicting with the exception thrown when a test is skipped.
Copy link
Member

@Naktibalda Naktibalda left a comment

Thanks, good job.
Please make a few more changes and it will be good to go.

composer.json Outdated
@@ -18,8 +18,9 @@
"php": ">=5.4.0 <8.0",
"ext-json": "*",
"ext-mbstring": "*",
"phpunit/phpunit": ">4.8.20 <6.0",
"phpunit/php-code-coverage": ">=2.2.4 <5.0",
"phpunit/phpunit": ">4.8.20 <6.1",
Copy link
Member

@Naktibalda Naktibalda Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<7.0 should be fine, right?

Copy link
Contributor Author

@MontealegreLuis MontealegreLuis Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure about this one, I read this comment on the issue #3995 (comment) and I tried to use a small range for the versions. If that's not a problem for you all, I can use a wider set of versions then.

composer.json Outdated
"phpunit/php-code-coverage": ">=2.2.4 <5.0",
"phpunit/phpunit": ">4.8.20 <6.1",
"phpunit/php-code-coverage": ">=2.2.4 < 4.1 || ^5.0",
"phpunit/phpunit-mock-objects": ">2.3 <3.5 || ^4.0",
Copy link
Member

@Naktibalda Naktibalda Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you got a reason to exclude php-code-coverage 4.1 and phpunit-mock-objects 3.5?

Copy link
Contributor Author

@MontealegreLuis MontealegreLuis Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reason for this one. What would be your preferred set of values for these versions?

Copy link
Member

@Naktibalda Naktibalda Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"phpunit/php-code-coverage": ">=2.2.4 < 6.0",
"phpunit/phpunit-mock-objects": ">2.3 <5.0",

composer.json Outdated
@@ -31,7 +32,7 @@
"symfony/css-selector": ">=2.7 <4.0",
"symfony/dom-crawler": ">=2.7.5 <4.0",
"behat/gherkin": "~4.4.0",
"sebastian/comparator": "~1.1",
"sebastian/comparator": ">1.1 <1.2 || ^2.0",
Copy link
Member

@Naktibalda Naktibalda Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, why are you excluding 1.2?

Copy link
Contributor Author

@MontealegreLuis MontealegreLuis Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just being cautious, as in the other ones. Please let me know what range would work better.

Copy link
Member

@Naktibalda Naktibalda Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

~1.1 actually allowed 1.2.* but you forbidden it.

">1.1 <3.0"

shim.php Outdated
// Add aliases for PHPUnit 6

namespace {
if (version_compare(PHP_VERSION, '7.0.0') >= 0 && class_exists('PHPUnit\Framework\TestCase')) {
Copy link
Member

@Naktibalda Naktibalda Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that if (!class_exists('PHPUnit_Framework_TestCase') && class_exists('PHPUnit\Framework\TestCase')) condition would be more accurate.

Copy link
Contributor Author

@MontealegreLuis MontealegreLuis Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found some weird behaviour when running the tests on PHP 5.6, which installs PHPUnit 5.7.*, I think. When I tried with the condition class_exists('PHPUnit\Framework\TestCase') it was returning true, which is incorrect, because this class is only present in PHPUnit 6, which can only be installed in PHP 7. I think it's because the class can be autoloaded. It's probably safe to check for PHP's version only.

Copy link
Member

@Naktibalda Naktibalda Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a forward compatibility layer, wasn't it? that's why you need to check PHPUnit_Framework_TestCase.

*/
protected $dispatcher;
// PHPUnit 6 definition for the TestListener class includes a new method `addWarning`
if (interface_exists('PHPUnit\Framework\TestListener')) {
Copy link
Member

@Naktibalda Naktibalda Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is an overkill to provide 2 versions of the class, simply add addWarning method, it doesn't matter if it is missing in the earlier versions of the interface.

Copy link
Contributor Author

@MontealegreLuis MontealegreLuis Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, now that I see it a second time, the solution was way simpler. I'll change it,

*/
public function setExpectedException($exception, $message = '', $code = null)
{
if (class_exists('PHPUnit\Framework\TestCase')) {
Copy link
Member

@Naktibalda Naktibalda Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

method_exists($this, 'expectException') is more accurate too, isn't it?

Copy link
Contributor Author

@MontealegreLuis MontealegreLuis Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're right I'll make the change.

if (class_exists('PHPUnit\Framework\TestCase')) {
$this->expectException($exception);
$message !== '' && $this->expectExceptionMessage($message);
$code !== null && $this->expectExceptionCode($code);
Copy link
Member

@Naktibalda Naktibalda Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't do this, use if statement instead:

if ($message !== '') {
    $this->expectExceptionMessage($message);
}

Copy link
Contributor Author

@MontealegreLuis MontealegreLuis Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, Ill change it.

* If the method exists (PHPUnit 5) forward the call to the parent class, otherwise
* call `expectException` instead (PHPUnit 6)
*/
public function setExpectedException($exception, $message = '', $code = null)
Copy link
Member

@Naktibalda Naktibalda Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, you added this method to 8 test files.
The most elegant solution would be to migrate tests (all?) from PhpUnit_Framework_Testcase to Codeception\Test\Unit, but more pragmatic solution would be to add a trait.

Copy link
Contributor Author

@MontealegreLuis MontealegreLuis Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some test classes not inheriting from Codeception\Test\Unit. That's why I didn't think of moving that method up. I think the trait is better. Unless you think that all those test should extend Codeception\Test\Unit.

Copy link
Member

@Naktibalda Naktibalda Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codeception\Test\Unit extends \PHPUnit_Framework_TestCase, so changing parent class should be straightforward. Please to update all tests if it doesn't cause any issues.

…plementations

* Remove the path to this file from nitpick.json
…nit.

* Remove duplicated method from all tests
* Check if the method expectException is present.
* Use if statement instead of the && operator to call the other 2 methods.
composer.json Outdated
"phpunit/php-code-coverage": ">=2.2.4 < 4.1 || ^5.0",
"phpunit/phpunit-mock-objects": ">2.3 <3.5 || ^4.0",
"phpunit/phpunit": ">4.8.20 <7.0",
"phpunit/php-code-coverage": ">=2.2.4 < 6.0",
Copy link
Member

@Naktibalda Naktibalda Apr 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please delete that space between < and 6

Copy link
Contributor Author

@MontealegreLuis MontealegreLuis Apr 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member

@Naktibalda Naktibalda left a comment

Thanks, good job.

@Naktibalda Naktibalda requested a review from DavertMik Apr 20, 2017
Copy link
Member

@DavertMik DavertMik left a comment

Awesome! Thanks for your work. I'd definitely like to see it in next release.

public function setExpectedException($exception, $message = '', $code = null)
{
if (method_exists($this, 'expectException')) {
$this->expectException($exception);
Copy link
Member

@DavertMik DavertMik Apr 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can show a deprecation method warning using https://github.com/Codeception/Codeception/blob/2.2/src/Codeception/Lib/Notification.php#L13

Does it make sense?

Copy link
Contributor Author

@MontealegreLuis MontealegreLuis Apr 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to all of you. I love this project! Yes, it makes sense. Is PHPUnit\Framework\TestCase::setExpectedException deprecated in favor of expectException, expectExceptionMessage, and expectExceptionCode a good message to use? This warning should only be shown when using PHPUnit 6 (not in the else of this condition)? Or it should be shown always (before the condition)?

Copy link
Member

@Naktibalda Naktibalda Apr 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DavertMik it would show a lot of warnings for Codeception test suite, and we can't switch until we drop support for PHPUnit 4.

Copy link
Member

@Naktibalda Naktibalda Apr 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MontealegreLuis I think that the warning should be shown if expectException exists.

Copy link
Contributor Author

@MontealegreLuis MontealegreLuis Apr 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok @Naktibalda will do that.

@orlangur
Copy link

@orlangur orlangur commented Apr 22, 2017

Very bad implementation approach 😄

By the way, any idea why test suite is not compatible with PHPUnit 5.4? https://travis-ci.org/orlangur/Codeception/jobs/208363815

@Naktibalda
Copy link
Member

@Naktibalda Naktibalda commented Apr 22, 2017

@orlangur Please try 5.7

@orlangur
Copy link

@orlangur orlangur commented Apr 22, 2017

@Naktibalda of course I can try... I'm trying to say that there are big holes in supported PHPUnit versions even before PHPUnit6 support with new hardcode.

So, it's better to drop PHPUnit 4, PHP <5.6 support and do some grooming instead of increasing fragility.

@Naktibalda
Copy link
Member

@Naktibalda Naktibalda commented Apr 22, 2017

Thanks for telling us.

We will do that in 2.3 or 3.0, whatever comes first.

*/
public function setExpectedException($exception, $message = '', $code = null)
{
if (method_exists($this, 'expectException')) {
Copy link
Member

@Naktibalda Naktibalda Apr 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code does not match comments.
The matching code would be

if (is_callable('parent::setExpectedException')) {
  parent::setExpectedException($exception, $message, $code);
} else {
 //...
}

Copy link
Contributor Author

@MontealegreLuis MontealegreLuis Apr 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change it.

Copy link
Contributor Author

@MontealegreLuis MontealegreLuis Apr 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one and the deprecation warning are done.

@Naktibalda
Copy link
Member

@Naktibalda Naktibalda commented Apr 22, 2017

@MontealegreLuis As you may have noticed, your work is only a temporary measure.
A large part of it will have to be removed once we drop support for PHP <5.6 and PHPUnit <5.7
All use cases of shimmed classes will be replaced with new names and all deprecated method calls will be replaced with new method calls.
Would you like to do that as well?

@MontealegreLuis
Copy link
Contributor Author

@MontealegreLuis MontealegreLuis commented Apr 22, 2017

Sure thing, I'd love to do that!!

@enumag
Copy link
Member

@enumag enumag commented Apr 24, 2017

This looks like a good compromise until Codeception 3 is ready. Looking forward to see this in a stable release.

@Naktibalda Naktibalda merged commit 503f1fe into Codeception:2.2 Apr 25, 2017
4 checks passed
@DavertMik
Copy link
Member

@DavertMik DavertMik commented Apr 26, 2017

This PR will be part of Codeception 2.3
There still can be issues with PHP 6,0 support so If they happen we encourage to restrict to phpunit ^5.7.0 manually in composer.json

@GwendolenLynch
Copy link
Contributor

@GwendolenLynch GwendolenLynch commented May 23, 2017

I see 2.3.0 was just released, but without the changes to composer.json to re-enable the higher constraint… will this be a 2.4 target now?

@Naktibalda
Copy link
Member

@Naktibalda Naktibalda commented May 23, 2017

@DavertMik This was a driver for 2.3.0 and you missed it. Can it go out in 2.3.1?

@DavertMik
Copy link
Member

@DavertMik DavertMik commented May 23, 2017

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

Successfully merging this pull request may close these issues.

None yet

7 participants