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

fix compatibility with PHPUnit error expectations #6461

Closed

Conversation

calvinalkan
Copy link
Contributor

I'm surprised that nobody noticed this before, but PHPUnits expectNotice() expectWarning(), expectError() methods dont seem to work at all.

This is because the ErrorHandler transforms all errors into exceptions instead of conditionally looking at the error code (like the PHPUnit error handler).

I created tests that prove this.

For deprecations, it's the same issue and I created a failing test. I don't now how to fix that tho because I don't understand how deprecations are currently handled here.

@calvinalkan calvinalkan force-pushed the fix/error-handling-expectations branch 2 times, most recently from d3743b5 to 36426ab Compare May 19, 2022 18:39
@calvinalkan
Copy link
Contributor Author

Tests are failing because of tests/unit/Codeception/Subscriber/ErrorHandlerTest.php::testShowsLocationOfWarning which is a test that IMO is only proving that things don't work with the PHPUnit API.

But maybe that's desired..?

@calvinalkan calvinalkan force-pushed the fix/error-handling-expectations branch from 36426ab to 1a9f31f Compare May 19, 2022 18:43
@Naktibalda
Copy link
Member

There methods are quite recent, they there introduced in PHPUnit 8.4,

}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.

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

}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.

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', '>=')) {
   ...
}


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', '<')) {

@Naktibalda
Copy link
Member

I implemented all proposed changes myself in #6469

@Naktibalda Naktibalda closed this Jun 10, 2022
@calvinalkan
Copy link
Contributor Author

@Naktibalda Great. Thanks a lot.

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

2 participants