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

DX: check deprecations exactly #7742

Merged
merged 3 commits into from
Jan 20, 2024
Merged

DX: check deprecations exactly #7742

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
36fcb86
DX: check deprecations exactly
kubawerlos Jan 15, 2024
ee24a33
DX: check deprecations exactly - order or count of expectation vs act…
keradus Jan 15, 2024
d87e4b1
Merge branch 'master' into 7610_recover
keradus Jan 20, 2024
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
1 change: 1 addition & 0 deletions tests/AutoReview/DescribeCommandTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public function testDescribeCommand(FixerFactory $factory, string $fixerName): v
{
// @TODO 4.0 Remove this expectation
$this->expectDeprecation('Rule set "@PER" is deprecated. Use "@PER-CS" instead.');
$this->expectDeprecation('Rule set "@PER:risky" is deprecated. Use "@PER-CS:risky" instead.');

$command = new DescribeCommand($factory);

Expand Down
1 change: 1 addition & 0 deletions tests/Console/Command/DescribeCommandTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ public function testExecuteOutput(string $expected, bool $expectedIsRegEx, bool
{
// @TODO 4.0 Remove this expectation
$this->expectDeprecation('Rule set "@PER" is deprecated. Use "@PER-CS" instead.');
$this->expectDeprecation('Rule set "@PER:risky" is deprecated. Use "@PER-CS:risky" instead.');

$actual = $this->execute(null !== $fixer ? $fixer->getName() : 'Foo/bar', $decorated, $fixer)->getDisplay(true);

Expand Down
1 change: 1 addition & 0 deletions tests/Console/ConfigurationResolverTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1222,6 +1222,7 @@ public function testDeprecatedRuleSetConfigured(string $ruleSet, array $successo
? 'No replacement available'
: sprintf('Use %s instead', Utils::naturalLanguageJoin($successors))
));

$config = new Config();
$config->setRules([$ruleSet => true]);
$config->setRiskyAllowed(true);
Expand Down
12 changes: 9 additions & 3 deletions tests/TestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,16 @@ abstract class TestCase extends BaseTestCase
protected function tearDown(): void
{
if (null !== $this->previouslyDefinedErrorHandler) {
foreach ($this->expectedDeprecations as $expectedDeprecation) {
self::assertContains($expectedDeprecation, $this->actualDeprecations);
}
$this->actualDeprecations = array_unique($this->actualDeprecations);
sort($this->actualDeprecations);
$this->expectedDeprecations = array_unique($this->expectedDeprecations);
sort($this->expectedDeprecations);
self::assertSame($this->expectedDeprecations, $this->actualDeprecations);
Comment on lines +38 to +42
Copy link
Member

Choose a reason for hiding this comment

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

My question still stands. After this change tests' result is conditioned by implementation. Let's say Foo::bar() triggers deprecation, if our logic uses this, then we expect the deprecation in tests. Why would we need to modify expectation in test when that method is used again in the tested logic? Does the fact that deprecated method is used multiple times change anything?

Copy link
Member Author

@keradus keradus Jan 19, 2024 •
edited

Choose a reason for hiding this comment

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

we care which deprecations were triggered (to ensure we do not rely on logic that was deprecated, without our explicit agreement to it), not how many times nor in which order
(thus, we have array_uniq and sort)

Copy link
Member

Choose a reason for hiding this comment

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

Hmmmm, probably I overlooked array_unique() 🤔. So how is this different from what we have currently? I believe with the previous loop it also was checking if all expected deprecations were caught, so do we want to check if there were more expectations than actual deprecations, or other way around?

Copy link
Member Author

Choose a reason for hiding this comment

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

current master is checking if all deprecations listed in expectDeprecation happen, but not checking if any more happen on top. This PR makes sure we check both.

When using Sf brigde, we checked for both.

Copy link
Member Author

Choose a reason for hiding this comment

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

ie master is not failing without https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/pull/7742/files#diff-80e18dd53b2dd28618278f183c30a01bc63e8aceec7508ab815ed2f124aa8f3dR58 , but this PR is.


restore_error_handler();
}

parent::tearDown();
}

final public function testNotDefiningConstructor(): void
Expand All @@ -54,6 +58,8 @@ final public function testNotDefiningConstructor(): void
}

/**
* Mark test to expect given deprecation. Order or repetition count of expected vs actual deprecation usage can vary, but result sets must be identical.
*
* @TODO change access to protected and pass the parameter when PHPUnit 9 support is dropped
*/
public function expectDeprecation(/* string $message */): void
Expand Down