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

DX: remove symfony/phpunit-bridge #7578

Merged
merged 3 commits into from Dec 21, 2023
Merged

DX: remove symfony/phpunit-bridge #7578

merged 3 commits into from Dec 21, 2023

Conversation

kubawerlos
Copy link
Contributor

Both listeners and expectDeprecation got removed in PHPUnit 10, so to keep checking for deprecations we need to have our implementation.

We can move to the event system that got introduced in PHPUnit 10 when we use PHPUnit 10+ (requires supporting PHP 8.1+ only).

@coveralls
Copy link

coveralls commented Dec 16, 2023

Coverage Status

coverage: 94.795%. remained the same
when pulling f0d242d on 6b7562617765726c6f73:dx_remove_symfony/phpunit-bridge
into 488440b on PHP-CS-Fixer:master.

@kubawerlos kubawerlos marked this pull request as ready for review December 20, 2023 20:47

protected function tearDown(): void
{
if (null !== $this->previouslyDefinedErrorHandler) {
Copy link
Member

Choose a reason for hiding this comment

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

is anything around sebastianbergmann/phpunit#5532 helping us to avoid local implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't able to figure anything out that would still work with PHP 7.4 and PHPUnit 9.

tests/TestCase.php Outdated Show resolved Hide resolved
tests/TestCase.php Outdated Show resolved Hide resolved
Comment on lines +38 to +40
foreach ($this->expectedDeprecations as $expectedDeprecation) {
self::assertContains($expectedDeprecation, $this->actualDeprecations);
}
Copy link
Member

Choose a reason for hiding this comment

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

what if count(actual) > count(expected) ?
we could miss some exceptions. I suggest to check them too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will do that in separate PR, it will look quite funny, I promise.


/**
* @TODO change access to protected and pass the parameter when PHPUnit 9 support is dropped
*/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
*/
* @param string $message
*/

possible ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ondrej says "no":

 ------ ----------------------------------------------------------
  Line   tests/TestCase.php
 ------ ----------------------------------------------------------
  60     PHPDoc tag @param references unknown parameter: $message
 ------ ----------------------------------------------------------

Copy link
Member

Choose a reason for hiding this comment

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

yeah, was afraid some sca will fail on "future param"

Copy link
Member

@keradus keradus left a comment

Choose a reason for hiding this comment

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

overall OK, left few minor comments

Co-authored-by: Dariusz Rumiński <dariusz.ruminski@gmail.com>
@kubawerlos kubawerlos enabled auto-merge (squash) December 21, 2023 12:48
@kubawerlos kubawerlos merged commit 4341133 into PHP-CS-Fixer:master Dec 21, 2023
24 checks passed
@kubawerlos kubawerlos deleted the dx_remove_symfony/phpunit-bridge branch December 21, 2023 12:54
danog pushed a commit to zoonru/PHP-CS-Fixer that referenced this pull request Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants