Benchmark how slow using composer require would be in kernel tests

Results
Basically I set up a 2 tests that do the same thing in a loop

1. Add a package
2. Get the installed packages
3. Remove the package

AddPackageBenchMarkTest does with the fixture manipulator plus $composerUtility->getInstalledPackages(); which uses composer php api
ComposerRequireBenchMarkTest does with the composer command plus $this->container->get('package_manager.composer_inspector')->getInstalledPackagesList($active_dir); which uses composer show

1. Commit where loop runs 5x and test runs 50x

   Output

   AddPackageBenchMarkTest Test run duration: 2 sec
   ComposerRequireBenchMarkTestTest run duration: 4 sec

2. Commit where loop runs 2x and test runs 20x

   Output

   AddPackageBenchMarkTest Test run duration: 5 sec
   ComposerRequireBenchMarkTestTest run duration: 18 sec

I think this shows our kernel test might run 3x as long or more.

In 📌 Update FixtureManipulator to work with InstalledPackagesList, real composer show command Fixed I showed that we can just update our FixtureManipulator class to work with a real composer show
and therefore work with $this->container->get('package_manager.composer_inspector')->getInstalledPackagesList($active_dir);

I think means for the means we should keep using the FixtureManipulator subclasses. The API we created in those classes I think later could be changed internally to actually do composer installs later if we want to.

(new FixtureManipulator())
      ->addPackage(
        [
          'name' => 'drupal/package_project_match',
          'type' => 'drupal-module',
          'install_path' => "$relative_projects_dir/package_project_match",
        ]
      )->commitChanges($fixture);

Doesn't actually tell you how it will be done.

We could even use a flag to use different implementation in some test or test runs if we someday are able to run a subset or all of our tests in test matrix with different versions of Composer

(I think you've flipped the links around? 😅)

I think this shows our kernel test might run 3x as long or more.

Can we really conclude that from this?

Running 50 kernel tests (which is more than we have today, but ballpark similar for sure 👍) with each of them doing 5 composer require calls (which is ballpark accurate too I think 👍).

But we're forgetting:

1. that a single composer require can install multiple packages, so e.g. the 6 FixtureManipulator::addPackage() calls in OverwriteExistingPackagesValidatorTest (representing >10% of all such calls!) would be replaced by a single call! Same thing for the 6 calls in StagedProjectsValidatorTest::testProjectsRemoved().
2. that addPackage() today is incorrect (that's why we have 📌 Update FixtureManipulator to work with InstalledPackagesList, real composer show command Fixed , to fix that), and that fixing it requires performing more I/O, which should decrease the difference.

So I'd like to see at least one more test: a test in which we still execute 50 kernel tests, but call composer require only 1 time in each — that should show how N addPackage() calls per test compare to a single composer require per test. So that'd answer that first point I made 🤓

While it is good to test this out because we might want to use more real composer calls in our test at sometime I am less convinced we need to do that now because

1. 📌 Update FixtureManipulator to work with InstalledPackagesList, real composer show command Fixed seems to be easier than I thought
2. 📌 Run `composer validate` after FixtureManipulator commits its changes Fixed will mean that we can be more confident that our manipulation is more realistic and will ensure it stays so
3. The test API in FixtureManipulator with methods addPackage, modifyPackage, removePackage could easily be updated later to actually perform real composer calls internally if we need to. So I think if we keep using FixtureManipulator we are not tied to just manipulating the composer files manually forever. The tests the use FixtureManipulator would hopefully have to change very little if we change the internals

   For a test that uses this really doesn't care how that package gets added it just needs the validator to be able to determine that it is installed.

   (new ActiveFixtureManipulator())
         ->addPackage([
           'name' => "drupal/dependency",
           'version' => '9.8.0',
           'type' => 'drupal-library',
         ])
         ->commitChanges();

   Currently we do this by changing files but we could change it use composer commands or even mock InstalledPackageList objects that return back the packages we set up.

For these reasons I bumping this back to normal priority

1. Running 50 kernel tests (which is more than we have today, but ballpark similar for sure 👍) with each of them doing 5 composer require calls (which is ballpark accurate too I think 👍).

   This probably very low estimate. This test has 1 method. We have 181 test method in our kernel tests. Many of these have data providers so they would called multiple times
   UPDATE: Just checked when all kernel test locally for package manager it is 307 , which I think includes data providers.

2. that addPackage() today is incorrect (that's why we have 📌 Update FixtureManipulator to work with InstalledPackagesList, real composer show command Fixed , to fix that), and that fixing it requires performing more I/O, which should decrease the difference.

   I think most of the new IO is done in that issue I pretty sure it will add very little time.
   checked the latest test run in #3343827 vs 3.0.x

   #3343827 appeared to take 2 minutes longer

   but looking at the executions

   Kernel tests
   #3343827 2 min 22 sec
   3.0.x 2 min 18 sec

   Function tests
   #3343827 2 min 7 sec
   3.0.x 2 min 4 sec

   Other test don't use the manipulators
   So about 30 seconds of the total 2 minutes difference could possibly be because of manipulators
   but also #3343827 does change 1 Validators to use the new InstalledPackagesList which is going to make composer process calls instead of using doing Composer PHP API calls. That process is slower and that updated validator `automatic_updates.staged_projects_validator` is only disabled in 1 test so in every other PreApplyEvent, which it subscribes to, in every other test these new composer show command will be run.

   So it likely the manipular changes affect the performance very little and much of the 30 seconds difference is just the new composer calls.
   Just uploaded a patch to test this out https://www.drupal.org/project/automatic_updates/issues/3343827#comment-... 📌 Update FixtureManipulator to work with InstalledPackagesList, real composer show command Fixed

   Also test I have just run all package manager kernel tests locally
   3.0.x - 7:42
   #3343827-without validator change - 7:02🤔
   So this problem just means something else is going with my composer, if was faster with the manipulation change

3. that a single composer require can install multiple packages, so e.g. the 6 FixtureManipulator::addPackage() calls in OverwriteExistingPackagesValidatorTest (representing >10% of all such calls!) would be replaced by a single call! Same thing for the 6 calls in StagedProjectsValidatorTest::testProjectsRemoved().

   so that made me realize to figure how composer call we could have it is better to track the calls to \Drupal\fixture_manipulator\FixtureManipulator::doCommitChanges to count 1 potential composer call for each group of calls to addPackage, modifyPackage, and removePackage

   will push up a start to this

create branch to 3343768-count-possible-composer-calls to show how could determine about how many real composer calls we would have to make if we wanted to switch to that.

FWIW with just Package Manager kernels testa locally this found 103

#6:

1. Wonderful! 🚀
2. Agreed on the importance of 📌 Run `composer validate` after FixtureManipulator commits its changes Fixed — thank you for creating that issue! 👍
3. Agreed that together with the previous point, we have a path forward for the future; we haven't locked ourselves in. 👍

RE #6's feedback on #5:

1. This probably very low estimate. This test has 1 method. We have 181 test method in our kernel tests.

   … but each of those require a setup, and that has overhead too.

2. 👍 Those numbers look very convincing! 4 seconds difference for kernel tests on a total of ~140 seconds is basically "noise", not a measurable difference.
   🤔 But what 2 minute difference are you talking about? DrupalCI run times themselves easily have a variance of multiple minutes! Grep the output for Starting environment , and you'll see that there's a 1-minute difference just because one env did not need to do docker pull and the other did! You showed that the performance difference for Kernel & Functional tests was effectively non-existent — and the same is true for Unit & Build tests. So the only possible conclusion I see: no performance difference.

#7: doesn't that mean that only ~103 times ~1 second would make tests only ~1 minute 40 seconds slower? 🤔

Do we still need this issue?

FixtureManipulator is now using nearly(?) entirely real Composer commands. As far as I know, this is moot and we did it.

I was thinking the same.

I'd like @tedbow to take a final look at this before closing it.

I think it is a performance hit but trying to update the composer metadata files directly was getting too complicated. If we need to make our test suite to be faster for core inclusion I think we should look elsewhere for improvements. for instance 📌 Optimize Composer calls in FixtureManipulator Closed: works as designed

If we need to find any more improvement tests like \Drupal\Tests\package_manager\Kernel\ComposerPatchesValidatorTest::testErrorDuringPreCreate instead of using a data provider could just have a loop resets the active directory to starting state at the start of each iteration and then runs all the different scenarios

I am not saying we should change testErrorDuringPreCreate I prefer to use the data provider. Just citing a quick example of a change we could make if we get push back on the test duration. While a change like that to testErrorDuringPreCreate would not be ideal it would much better than going back to editing composer metadata file directly.

I think it is a performance hit but trying to update the composer metadata files directly was getting too complicated.

💯

I'm glad and relieved that @phenaproxima, @tedbow and I are all on the same page 😊

Automatically closed - issue fixed for 2 weeks with no activity.