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

Implement expectUserDeprecationMessage() and expectUserDeprecationMessageMatches() to expect E_USER_DEPRECATED issues #5605

Closed
wants to merge 5 commits into from

Conversation

sebastianbergmann
Copy link
Owner

@sebastianbergmann sebastianbergmann commented Dec 8, 2023

This implements the expectUserDeprecationMessage() and expectUserDeprecationMessageMatches() methods for expecting E_USER_DEPRECATED issues.

The test method must be attributed with #[PHPUnit\Framework\Attributes\IgnoreDeprecations] in order to not have the expected deprecation(s) trigger PHPUnit's default handling of E_USER_DEPRECATED issues.

Multiple deprecations can be expected in the same test by calling expectUserDeprecationMessage() and expectUserDeprecationMessageMatches() multiple times.

@stof This is my first attempt to implement what we discussed yesterday. Would this help you?

@sebastianbergmann sebastianbergmann added the type/enhancement A new idea that should be implemented label Dec 8, 2023
Copy link

codecov bot commented Dec 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d6affa8) 88.48% compared to head (7646137) 88.51%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5605      +/-   ##
============================================
+ Coverage     88.48%   88.51%   +0.03%     
- Complexity     6272     6293      +21     
============================================
  Files           668      673       +5     
  Lines         20068    20125      +57     
============================================
+ Hits          17757    17814      +57     
  Misses         2311     2311              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sebastianbergmann sebastianbergmann force-pushed the expect-user-deprecation branch 2 times, most recently from 9233648 to ec415a7 Compare December 14, 2023 08:27
@stof
Copy link
Contributor

stof commented Dec 14, 2023

The feature of the symfony/phpunit-bridge actually allows defining multiple expected deprecations in the same test. This does not seem to be supported by your feature.

@stof
Copy link
Contributor

stof commented Dec 14, 2023

Btw, is it actually true that PHP issues have non-empty-string as their message ? Isn't it possible in PHP to pass an empty string to trigger_error ?

@sebastianbergmann
Copy link
Owner Author

The feature of the symfony/phpunit-bridge actually allows defining multiple expected deprecations in the same test. This does not seem to be supported by your feature.

Would this help?

$this->expectUserDeprecationMessage(
    [
        'deprecation message',
        'another deprecation message',
    ]
);

@sebastianbergmann
Copy link
Owner Author

Isn't it possible in PHP to pass an empty string to trigger_error?

That may be the case, but is outside the scope of this PR.

@stof
Copy link
Contributor

stof commented Dec 14, 2023

Well, I asked that question because this PR is adding those @psalm-param non-empty-string on all those APIs.

@stof
Copy link
Contributor

stof commented Dec 14, 2023

Would this help?

$this->expectUserDeprecationMessage(
    [
        'deprecation message',
        'another deprecation message',
    ]
);

This could work. But wouldn't it be possible to call the method multiple times instead ? This would more closely match our existing API (and maybe allow us to provide a compatibility layer for the migration)

@sebastianbergmann
Copy link
Owner Author

Well, I asked that question because this PR is adding those @psalm-param non-empty-string on all those APIs.

No, it is not. Those changes were done in main. No idea why GitHub thinks that they are part of this PR.

@sebastianbergmann
Copy link
Owner Author

sebastianbergmann commented Dec 14, 2023

But wouldn't it be possible to call the method multiple times instead?

I already started implementing this instead of what I proposed :)

@sebastianbergmann
Copy link
Owner Author

But wouldn't it be possible to call the method multiple times instead?

I already started implemented that instead of what I proposed :)

Pushed.

@sebastianbergmann
Copy link
Owner Author

Well, I asked that question because this PR is adding those @psalm-param non-empty-string on all those APIs.

No, it is not. Those changes were done in main. No idea why GitHub thinks that they are part of this PR.

Force-pushed the latest changes and now this PR only shows changes exclusive to the respective branch.

@sebastianbergmann
Copy link
Owner Author

@jrfnl I see that you reacted with 🎉 to this. When this is merged, do you think that #[WithoutErrorHandler] should be deprecated and later removed as proposed by @mvorisek in #5592?

src/Framework/TestCase.php Outdated Show resolved Hide resolved
@jrfnl
Copy link
Contributor

jrfnl commented Dec 15, 2023

@sebastianbergmann Thanks for asking.

@jrfnl I see that you reacted with 🎉 to this.

Correct. I did so as I'm seeing that a number of code bases are not making the move to support PHPUnit 10 largely because they can no longer test their own deprecations (and to a much lesser extend notices and warnings).

When this is merged, do you think that #[WithoutErrorHandler] should be deprecated and later removed as proposed by @mvorisek in #5592?

I don't see this as directly related to each other ?

The #[WithoutErrorHandler] was introduced for error_get_last() and yes, it could be used to implement a custom error handler to allow for still testing deprecations and such, but the original use case for this, was code using error_get_last() in the source code of a code base to get an error message (from a stream error) to use in an Exception. Not to work around the removal of the expect*() methods.

@sebastianbergmann
Copy link
Owner Author

@stof Thank you for reviewing this!

@sebastianbergmann sebastianbergmann deleted the expect-user-deprecation branch December 18, 2023 05:07
@sebastianbergmann sebastianbergmann restored the expect-user-deprecation branch December 22, 2023 07:34
@sebastianbergmann sebastianbergmann deleted the expect-user-deprecation branch December 22, 2023 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement A new idea that should be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants