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

dev/core#3852 - Classload extension test folders during tests #24549

Merged
merged 1 commit into from
Sep 18, 2022

Conversation

demeritcowboy
Copy link
Contributor

@demeritcowboy demeritcowboy commented Sep 17, 2022

Overview

https://lab.civicrm.org/dev/core/-/issues/3852

Before

Some extension actions like install fail if class scanning tries to scan them during tests.

After

Technical Details

Scanning runs more often after #24276, and the test folders weren't included in classloading.

Comments

I could include the unit test from the lab ticket, but it's a bit of a goofy test. Trying to think of something more likely to see in real life.
For what it's worth the drupal/mink tests now pass with this applied: e.g. https://github.com/SemperIT/CiviCARROT/actions/runs/3073761641/jobs/4966094386#step:19:4

@civibot
Copy link

civibot bot commented Sep 17, 2022

(Standard links)

@civibot civibot bot added the master label Sep 17, 2022
@@ -164,6 +164,9 @@ private static function loadExtension(\Composer\Autoload\ClassLoader $loader, CR

case 'psr4':
$loader->addPsr4($mapping['prefix'], $path . '/' . $mapping['path']);
if (defined('CIVICRM_TEST')) {
$loader->addPsr4($mapping['prefix'], $path . '/tests/phpunit/' . $mapping['path']);
Copy link
Contributor

Choose a reason for hiding this comment

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

@demeritcowboy just wondering if we should be testing for the presence of the test folder as well but probably doesn't matter that much

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting question - yeah probably. Composer does this, so if the array is smaller to begin with it should be more efficient, trading one is_dir call here for several file_exists later. And then it would make sense to do it for non-test folders too.

Let's add it. For unscientific testing purposes the first one "Took 2 hr 31 min on test-1".

@colemanw
Copy link
Member

Latest run "Took 2 hr 37 min on test-1" - not a significant difference.

@colemanw colemanw merged commit 8d63fde into civicrm:master Sep 18, 2022
@demeritcowboy
Copy link
Contributor Author

Yeah it would surprise me - here we're talking about <10 extensions and about half DO have Civi-namespace tests anyway.

@demeritcowboy demeritcowboy deleted the load branch September 18, 2022 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants