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.

Sign up for GitHub

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

Jump to bottom

fix compatibility with PHPUnit error expectations #6461

Closed
wants to merge 1 commit into from
Closed

fix compatibility with PHPUnit error expectations #6461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/Codeception/Subscriber/ErrorHandler.php
View file
Open in desktop
Expand Up @@ -4,8 +4,15 @@
use Codeception\Event\SuiteEvent;
use Codeception\Events;
use Codeception\Lib\Notification;
use PHPUnit\Framework\Error\Error;
use PHPUnit\Framework\Error\Notice;
use PHPUnit\Framework\Error\Warning;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;

use const E_USER_ERROR;
use const E_USER_NOTICE;
use const E_USER_WARNING;

class ErrorHandler implements EventSubscriberInterface
{
use Shared\StaticEvents;
Expand Down Expand Up @@ -80,6 +87,14 @@ public function errorHandler($errno, $errstr, $errfile, $errline, $context = arr
return false;
}

if($errno === E_USER_NOTICE) {
throw new Notice($errstr, $errno, $errfile, $errline);
}elseif ($errno === E_USER_WARNING) {
throw new Warning($errstr, $errno, $errfile, $errline);
}elseif ($errno === E_USER_ERROR) {
throw new Error($errstr, $errno, $errfile, $errline);
}

Copy link
Member

Choose a reason for hiding this comment

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

To get this change into Codeception 4, it must be compatible with PHPUnit 5.7.
Classes you used here were renamed in PHPUnit 6.0, so this code would cause fatal error.
Please add some test that runs on all versions of PHP.
Then add class aliases to https://github.com/Codeception/phpunit-wrapper/blob/6.0/src/shim.php

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Naktibalda What about the deprecation stuff?

Codeception should support expectDeprecation aswell.

But I don't understand what is done there currently. Looks like something symfony-related.

Copy link
Member

Choose a reason for hiding this comment

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

Codeception doesn't have to support any of this.

Deprecations are counted and summary is printed below test results:
image

Copy link
Member

Choose a reason for hiding this comment

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

There is no equivalent of Notice class in PHPUnit 5.7.

I think that it would be the best to wrap this section with

if (version_compare(\PHPUnit\Runner\Version::id(), '8.4.0', '>=')) {
   ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Codeception doesn't have to support any of this.

Deprecations are counted and summary is printed below test results: image

@Naktibalda But doesn't this just show the deprecations that occurred while running the test?

What I'm talking about (and what PHPUnit supports in the TestCase class) is making assertions about deprecations.

class Foo {

   public function foo(): {
     trigger_error('foo is deprecated', E_USER_DEPRECATED);
   }

}

class FooTest extends Codeception\Unit {

  public function test_that_foo_triggers_deprecation() {
  
     $this->expectDeprecation('foo is deprecated');
     
     (new Foo())->foo();
     
  }


}

Copy link
Member

Choose a reason for hiding this comment

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

Since 8.5.21 PHPUnit doesn't convert deprecations to exceptions by default, it requires setting configuration option.

https://github.com/sebastianbergmann/phpunit/blob/8.5/ChangeLog-8.5.md#8521---2021-09-25

You could implement convert_deprecations_to_exceptions option too.

$relativePath = codecept_relative_path($errfile);
throw new \PHPUnit\Framework\Exception("$errstr at $relativePath:$errline", $errno);
}
Expand Down
52 changes: 52 additions & 0 deletions tests/cli/ErrorExpectationsCest.php
View file
Open in desktop
@@ -0,0 +1,52 @@
<?php

class ErrorExpectationsCest
{

public function _before(\CliGuy $I)
{
$I->amInPath('tests/data/error_handling');
}

public function expectNoticeWorks(\CliGuy $I)
{
$this->skipIfNot72($I);

$I->executeCommand('run tests/unit/ErrorExceptionTest.php:test_notice');
$I->seeInShellOutput("OK (");
}

public function expectWarningWorks(\CliGuy $I)
{
$this->skipIfNot72($I);

$I->executeCommand('run tests/unit/ErrorExceptionTest.php:test_warning');
$I->seeInShellOutput('OK (');
}

public function expectErrorWorks(\CliGuy $I)
{
$this->skipIfNot72($I);

$I->executeCommand('run tests/unit/ErrorExceptionTest.php:test_error');
$I->seeInShellOutput('OK (');
}

public function expectDeprecationWorks(\CliGuy $I)
{
$this->skipIfNot72($I);
$I->markTestSkipped('This test is just to reproduce that is doesnt work. It will fail because nothing has been implemented');

$I->executeCommand('run tests/unit/ErrorExceptionTest.php:test_deprecation');
$I->seeInShellOutput('OK (');
}

private function skipIfNot72(CliGuy $I)
{
if(PHP_VERSION_ID < 70200) {
Copy link
Member

Choose a reason for hiding this comment

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

And change this condition to

if (version_compare(\PHPUnit\Runner\Version::id(), '8.4.0', '<')) {

$I->markTestSkipped('expectXXX is only available on 7.2+');
}
}

}

13 changes: 13 additions & 0 deletions tests/data/error_handling/codeception.yml
View file
Open in desktop
@@ -0,0 +1,13 @@
actor: Tester
paths:
tests: tests
log: tests/_output
data: tests/_data
support: tests/_support
envs: tests/_envs
suites:
unit:
path: .
modules:
enabled:
- Asserts
Empty file added tests/data/error_handling/tests/_support/.gitkeep
View file
Open in desktop
Empty file.
35 changes: 35 additions & 0 deletions tests/data/error_handling/tests/unit/ErrorExceptionTest.php
View file
Open in desktop
@@ -0,0 +1,35 @@
<?php

class ErrorExceptionTest extends \PHPUnit\Framework\TestCase
{

public function test_notice()
Copy link
Member

Choose a reason for hiding this comment

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

testNotice

{
$this->expectNotice();
$this->expectNoticeMessage('foobar');
trigger_error('foobar', E_USER_NOTICE);
}

public function test_warning()
{
$this->expectWarning();
$this->expectWarningMessage('foobar');
trigger_error('foobar', E_USER_WARNING);
}

public function test_error()
{
$this->expectError();
$this->expectErrorMessage('foobar');
trigger_error('foobar', E_USER_ERROR);
}

public function test_deprecation()
{
// This test fails.
$this->expectDeprecation();
$this->expectDeprecationMessage('foobar');
trigger_error('foobar', E_USER_DEPRECATED);
}

}
2 changes: 2 additions & 0 deletions tests/unit/Codeception/Subscriber/ErrorHandlerTest.php
View file
Open in desktop
Expand Up @@ -38,6 +38,8 @@ public function testDeprecationMessagesRespectErrorLevelSetting()

public function testShowsLocationOfWarning()
{
$this->markTestSkipped('Skipped to see if all tests pass');

if (PHP_MAJOR_VERSION === 5) {
$this->expectException(\PHPUnit_Framework_Exception::class);
} else {
Expand Down