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

[NFC/Unit Tests] Use phpunit polyfill for deprecated assertRegExp #25635

Closed
wants to merge 1 commit into from

Conversation

demeritcowboy
Copy link
Contributor

Overview

assertRegExp is deprecated in phpunit 9, but the replacement isn't available yet in phpunit 8. The polyfill fills the gap.

Before

Can't replace deprecated functions yet.

After

Can replace deprecated functions.

Technical Details

The meat of it is the use Yoast\PHPUnitPolyfills\Polyfills\AssertionRenames; in CiviUnitTestCase. The rest is just find/replace.

I expect this will cause (more) tests to fail in the drupal 8 matrix. The way the build works there it doesn't pull in the dev dependencies. I won't rant about composer here just will say that civi is a subproject in that matrix not the root, so dev dependencies need to be pulled in by the build script.

Comments

The original diff I was using for drupal 10 tests also had a fix in CRM_Utils_System::fixUrl for null input, but I've left that out since it doesn't really belong here, and I can't remember offhand what triggered it. I think I was going to go back and try to find out what was passing in null for the url. I don't think it will come up in the run here.

@civibot
Copy link

civibot bot commented Feb 21, 2023

(Standard links)

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@demeritcowboy
Copy link
Contributor Author

demeritcowboy commented Feb 21, 2023

Those were real fails, mostly. I probably only ran a sample with phpunit8 when I originally set it up just to check that it could work. There are things like tests which don't extend TestCase which might need some massaging if we continue with this route (see https://github.com/Yoast/PHPUnit-Polyfills#supported-ways-of-calling-the-assertions). And then some which I haven't had a chance to look what the problem is.

I suppose all the assertRegExp's could just be rewritten as $this->assertEquals(1, preg_match(...thing...)) as an alternate way of sidestepping the whole problem.

@demeritcowboy demeritcowboy added the needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state label Feb 21, 2023
@totten
Copy link
Member

totten commented Feb 21, 2023

I suppose all the assertRegExp's could just be rewritten as $this->assertEquals(1, preg_match(...thing...)) as an alternate way of sidestepping the whole problem.

One could also add a new method to do the exact same thing... but with a different name! Many premium names are available for the choosing, like assertMatchingExpression(), assertThatTheRegularExpressionIsMatched(), assertMatchForExpression(), validateWithPattern(), applyPhpunitAssertionForRegularExpression(), and assertTextualFiniteStateMachineTerminatesPositively(). These names and more can be yours - all you have to do is join the PHPUnit NameGameExtravaganza.

@demeritcowboy
Copy link
Contributor Author

Hehe.
How about assertRegExp?

@totten
Copy link
Member

totten commented Feb 21, 2023

😄

Fun fact - that's actually what I did on another repo: https://github.com/totten/git-scan/blob/2022-07-15/tests/GitScan/GitScanTestCase.php#L103-L121

(Edit: Though I'm sure the polyfill projects have more depth than ^^^...)

@eileenmcnaughton
Copy link
Contributor

UG - I added these to civicrm vendor - maybe they needed to go in buildkit?

@demeritcowboy
Copy link
Contributor Author

just reopening to get the list of fails again

@demeritcowboy
Copy link
Contributor Author

Put the list at https://lab.civicrm.org/dev/core/-/issues/4188

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master needs-work-not-review-ready Submitter required to take action, will be closed after 1-2 weeks in this state
Projects
None yet
4 participants