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: check deprecations exactly #7610

Closed
wants to merge 4 commits into from
Closed

DX: check deprecations exactly #7610

wants to merge 4 commits into from

Conversation

kubawerlos
Copy link
Contributor

@coveralls
Copy link

coveralls commented Dec 21, 2023

Coverage Status

coverage: 94.795%. remained the same
when pulling 40a4ec9 on 6b7562617765726c6f73:dx_check_deprecations_exactly
into 8058b05 on PHP-CS-Fixer:master.

Comment on lines 145 to 147
$this->expectDeprecation($deprecation);
$this->expectDeprecation($deprecation);
$this->expectDeprecation($deprecation);
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to expect exactly the same deprecation message multiple times? I would rather want to ensure that every expected deprecation message happens at least once. What is the gain from checking the exact amount of messages?

Copy link
Member

Choose a reason for hiding this comment

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

👍

we care to detect that there is deprecation. There is no value in having same deprecation msg once or 10 times.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it make sense to expect exactly the same deprecation message multiple times?

Of course not, I just wanted to show that if we would want to check the exact count (as suggested in #7578 (comment)) there would be multiple expectations of the same message.

Copy link
Member

Choose a reason for hiding this comment

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

that was totally not my point @kubawerlos .
my point was what if yet another deprecation would happen. let say, the "unexpected" one - we should fail. now we silently move on.

foreach ($this->expectedDeprecations as $expectedDeprecation) {
self::assertContains($expectedDeprecation, $this->actualDeprecations);
}
self::assertSame($this->expectedDeprecations, $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.

this way you depends on order of triggered deprecation. if someone done DX refactoring of method, method works exactly same way, but deprecations will get triggered in other order - this assertion will fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that bad? SymfonyTestsListener needs expectDeprecation called in order they are triggered.

Copy link
Member

Choose a reason for hiding this comment

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

Symfony's implementation aside - why order of deprecations should be interesting? In my opinion you either expect some deprecation or not, it does not matter if the executed code triggers it at the beginning or in the end.

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 have no problem with not caring about the order (which is how it is after the latest commit), and at the same time, I can imagine someone wanting to check the order in what deprecations were triggered (like maybe from least detailed to most detailed).

@6b7562617765726c6f73 6b7562617765726c6f73 closed this by deleting the head repository Dec 22, 2023
@keradus
Copy link
Member

keradus commented Jan 15, 2024

recovered #7742

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

5 participants