Skip to content

[PhpUnitBridge] Add ability to set a baseline for deprecation testing #37733

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
Merged
fabpot merged 1 commit into from
Oct 7, 2020
Merged

[PhpUnitBridge] Add ability to set a baseline for deprecation testing #37733

fabpot merged 1 commit into from
Oct 7, 2020

Conversation

@alexpott
Copy link
Contributor

@alexpott alexpott commented Aug 3, 2020 •
edited by nicolas-grekas
Loading

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #37715, #34496
License MIT
Doc PR symfony/symfony-docs#...

This PR allows you set new options for SYMFONY_DEPRECATIONS_HELPER env var:

  • generateBaseline - if this is set to TRUE any deprecations that occur will be written to the file defined in the baselineFile option
  • baselineFile a path to a file that contains a json encoded array of deprecations that will be skipped.

Questions

  • If you set generateBaseline without also setting baselineFile an exception is thrown. We could use a default filename if one is not provided (like PHPStan).
  • How much error checking should we do around the baselineFile variable - should we check if it is readable or should we rely on file_get_contents()?

Still @todo

Add proper end-to-end testing using a .phpt test

stof
stof reviewed Aug 4, 2020
View reviewed changes
stof
stof reviewed Aug 5, 2020
View reviewed changes
ogizanagi
ogizanagi reviewed Aug 12, 2020
View reviewed changes
src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler/Configuration.php Outdated
*/
public function isBaselineDeprecation(Deprecation $deprecation)
{
$result = \in_array($deprecation->getMessage(), $this->baselineDeprecations, true);
Copy link
Contributor

@ogizanagi ogizanagi Aug 12, 2020

Choose a reason for hiding this comment

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

So, we'll ignore any new occurence for a deprecation message.
Shouldn't we account for the number of occurrences for the baseline? As well as the class::method / triggering file?

The way this feature works in PHPStan is accounting at least for occurrences per class: https://medium.com/@ondrejmirtes/phpstans-baseline-feature-lets-you-hold-new-code-to-a-higher-standard-e77d815a5dff

Copy link
Member

@nicolas-grekas nicolas-grekas Sep 24, 2020

Choose a reason for hiding this comment

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

I agree with @ogizanagi, checking the count would be great.

@fabpot fabpot added this to the next milestone Aug 23, 2020
@nicolas-grekas nicolas-grekas changed the title Add ability to set a baseline for deprecation testing [PhpUnitBridge] Add ability to set a baseline for deprecation testing Sep 24, 2020
nicolas-grekas
nicolas-grekas reviewed Sep 24, 2020
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Thanks! Here are some comments.

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler/Configuration.php Outdated
*/
public function isBaselineDeprecation(Deprecation $deprecation)
{
$result = \in_array($deprecation->getMessage(), $this->baselineDeprecations, true);
Copy link
Member

@nicolas-grekas nicolas-grekas Sep 24, 2020

Choose a reason for hiding this comment

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

I agree with @ogizanagi, checking the count would be great.

@alexpott alexpott changed the base branch from 5.1 to master September 30, 2020 10:36
nicolas-grekas
nicolas-grekas reviewed Sep 30, 2020
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

One CS issue + OK to fix @ogizanagi's comment?

As a reminder:

So, we'll ignore any new occurence for a deprecation message.
Shouldn't we account for the number of occurrences for the baseline? As well as the class::method / triggering file?

The way this feature works in PHPStan is accounting at least for occurrences per class: medium.com/@ondrejmirtes/phpstans-baseline-feature-lets-you-hold-new-code-to-a-higher-standard-e77d815a5dff

src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler/Configuration.php Outdated
$this->verboseOutput[$group] = (bool) $status;
}

if ($generateBaseline && !($baselineFile)) {
Copy link
Member

@nicolas-grekas nicolas-grekas Sep 30, 2020

Choose a reason for hiding this comment

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

extra brackets should be removed :)

@lyrixx lyrixx removed their request for review September 30, 2020 16:07
@fabpot
Copy link
Member

fabpot commented Oct 7, 2020

@alexpott Still interested in finishing this PR?

@alexpott alexpott force-pushed the deprecation-baseline branch from f4c7577 to 3836618 Compare October 7, 2020 09:17
@alexpott
Copy link
Contributor Author

alexpott commented Oct 7, 2020

@fabpot @nicolas-grekas @ogizanagi I've added the deprecation location and counting feature. It's pretty ugly but it works. Open to suggestions for improvements :)

@fabpot fabpot changed the base branch from master to 5.x October 7, 2020 11:07
@fabpot fabpot force-pushed the deprecation-baseline branch from 3836618 to 483236f Compare October 7, 2020 11:07
@fabpot
Copy link
Member

fabpot commented Oct 7, 2020

Thank you @alexpott.

@fabpot fabpot mentioned this pull request Oct 7, 2020
Closed
@fabpot fabpot merged commit 559ebe3 into symfony:5.x Oct 7, 2020
@nicolas-grekas nicolas-grekas modified the milestones: 5.x, 5.2 Oct 7, 2020
@derrabus derrabus mentioned this pull request Oct 10, 2020
Merged
@fabpot fabpot mentioned this pull request Oct 14, 2020
Merged
@ruudk ruudk mentioned this pull request Apr 4, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stof stof stof left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@dunglas dunglas Awaiting requested review from dunglas

@jderusse jderusse Awaiting requested review from jderusse

@sroze sroze Awaiting requested review from sroze

@xabbuh xabbuh Awaiting requested review from xabbuh

@ogizanagi ogizanagi Awaiting requested review from ogizanagi

Assignees

No one assigned

Labels

Feature Status: Needs Review

Projects

None yet

Milestone

5.2

Development

Successfully merging this pull request may close these issues.

Allow listed deprecations to be skipped

6 participants

@alexpott @fabpot @nicolas-grekas @stof @ogizanagi @carsonbot