Reduce "Initial Tests Run" stage time by running only covering test files #1919

maks-rafalko · 2024-03-17T18:12:24Z

This PR:

Adds new feature to speed up "Initial Tests Run" stage and as a result - speed up Infection execution by a lot
Covered by tests
Doc PR: Document new --map-source-class-to-test option site#259

Originally requested here: #1451

Idea explanation

If we have X tests files and run Infection with for example infection --git-diff-lines and only 1 file is updated/added - it does not make sense to run all the X tests to generate coverage if we know that only 1 file of X covers the source code file.

In practice, PHPUnit even has different methods that help define which test file covers which class:

requireCoverageMetadata configuration attribute (previously forceCoversAnnotation="true")
CoversClass attribute / @covers annotation and others

With the following code:

use use PHPUnit\Framework\Attributes\CoversClass;

#[CoversClass(Calculator::class)]
class CalculatorTest extends TestCase {}

we can say that Calculator class is covered by CalculatorTest test class, and vice-versa. So, from Infection point of view, when we use --git-diff-lines and someone creates PR with updated Calculator class, we can run only CalculatorTest and skip other X test files (as they anyway don't add any useful coverage), saving a lot of time.

Things to note

Implemented feature doesn't change default Infection behavior and can be enabled by adding --map-source-class-to-test, so it's opt-in
implementation is intentionally simple but already gives a huge win, see below. But I designed it in such a way that it can be extended to map source class to test class with more complex strategies

Example with PHP-CS-Fixer

After maintainers of PHP-CS-Fixer decided to integrate Infeciton into their pipeline, I was analysing why "Initial Tests Run" stage takes so much time when even only 1 file is changed in PR.

It turned out, that PHP-CS-Fixer has ~24,000 unit tests, that all are executed when we change 1 source file.

At the same time, PHP-CS-Fixer has @covers annotation on all test classes, and we can 100% map which test class covers which source class, and run only covering test classes instead of all ~24,000 tests.

Before:

FAST_LINT_TEST_CASES=1 infection.phar --git-diff-lines

    ____      ____          __  _
   /  _/___  / __/__  _____/ /_(_)___  ____
   / // __ \/ /_/ _ \/ ___/ __/ / __ \/ __ \
 _/ // / / / __/  __/ /__/ /_/ / /_/ / / / /
/___/_/ /_/_/  \___/\___/\__/_/\____/_/ /_/

#StandWithUkraine

Infection - PHP Mutation Testing Framework version dev-master

Running initial test suite...

PHPUnit version: 10.5.13

 24768 [============================] 4 mins

After:

FAST_LINT_TEST_CASES=1 infection.phar --git-diff-lines --map-source-class-to-test

    ____      ____          __  _
   /  _/___  / __/__  _____/ /_(_)___  ____
   / // __ \/ /_/ _ \/ ___/ __/ / __ \/ __ \
 _/ // / / / __/  __/ /__/ /_/ / /_/ / / / /
/___/_/ /_/_/  \___/\___/\__/_/\____/_/ /_/

#StandWithUkraine

Infection - PHP Mutation Testing Framework version dev-master

Running initial test suite...

PHPUnit version: 10.5.13

   41 [============================] 12 secs

Difference:

- 24768 [============================] 4 mins
+    41 [============================] 12 secs

So, for a simple PR with 1 file chaged, now "Initital Tests Run" runs 41 tests by 12 seconds instead of 24,768 tests by 4 minutes.

Technical comments

I said the current implementation is intentionally simple, meaning the following: currently, we are not finding which test class covers particular changed file by finding all the CoversClass attributes / @covers annotations, but just make an assumption that if you have Calculator class, then test that covers it is named CalculatorTest.

Actually this is true for Infection, this is how we map Source Class to Test Class:

infection/tests/phpunit/AutoReview/SourceTestClassNameScheme.php

Lines 57 to 64 in eb4842d

    
           public static function getTestClassName(string $sourceClassName): string 
        
           { 
        
               if (strpos($sourceClassName, 'Infection\\Tests') !== false) { 
        
                   return $sourceClassName . 'Test'; 
        
               } 
        
               return preg_replace('/Infection/', 'Infection\\Tests', $sourceClassName, 1) . 'Test'; 
        
           }

and this is also true for PHP-CS-Fixer, they also have such mapping from Source Class to Test Class:

https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/blob/3b3bfe90fe8d5d920328a1f36288d2dfacf5d6b3/tests/AutoReview/ProjectCodeTest.php#L73-L78

Since we now know that Calculator class is covered by CalculatorTest test class, we can very simply build a --filter option for "Initial Tests Run" stage, just by configuring regex:

--filter='CalculatorTest|SomeOtherClassTest'

I've named this "strategy" of mapping source class to test class as simple. Later, if needed, we can implement more complex non-standard strategies, e.g. the one that will phisically parse all files from tests folder and find those classes that have CoversClass(Calculator::class), if we need more strict mapping.

But I intentionally didn't do it at this point.

…e the time for Initial Tests Run

maks-rafalko · 2024-03-17T18:17:51Z

src/TestFramework/PhpUnit/CommandLine/ArgumentsAndOptionsBuilder.php

+                array_map(
+                    static fn (SplFileInfo $sourceFile): string => sprintf('%sTest', $sourceFile->getBasename('.' . $sourceFile->getExtension())),
+                    $this->filteredSourceFilesToMutate,
+                )


These 10 lines actually the real implementation of the feature that needs attention.

as I wrote in the description, this is a simple strategy that maps SourceClass to SourceClassTest

…ble "Initial Tests Run" test files filtering Implements #1451

src/TestFramework/PhpUnit/CommandLine/ArgumentsAndOptionsBuilder.php

sidz · 2024-03-18T08:00:30Z

src/TestFramework/MapSourceClassToTestStrategy.php

+/**
+ * @internal
+ */
+final class MapSourceClassToTestStrategy


We could use enums for this purposes.

And to avoid nullable values technically we could use NONE as an option instead of NULL

Yeah, I thought about it. But intentionally didn't use Enums because we have all the "enum-like" classes as usual classes with constants. So it would be strange to have somewhere enums and somewhere classes with constants - I decided to be consistent here.

If we decide to use enums - we can replace all the classes at once as a separate PR. Currently, 0 enums are in the code base.

src/Command/RunCommand.php

theofidry · 2024-03-18T12:26:55Z

src/TestFramework/Factory.php

-            iterator_to_array($this->sourceFileFilter->filter($this->infectionConfig->getSourceFiles()))
-        ));
+        /** @var list<SplFileInfo> $files */
+        $files = iterator_to_array($this->sourceFileFilter->filter($this->infectionConfig->getSourceFiles()));


could this change (that is a bit noisy as it is affecting a few method signatures / phpstan / mutations) be extracted?

theofidry · 2024-03-18T12:27:08Z

src/TestFramework/Factory.php

-            static fn (SplFileInfo $file) => $file->getRealPath(),
-            iterator_to_array($this->sourceFileFilter->filter($this->infectionConfig->getSourceFiles()))
-        ));
+        /** @var list<SplFileInfo> $files */


is that phpdoc really necessary?

short answer: yes, because deeper in the stack we have return type iterable<SplFileInfo|Trace> and here we need to specify it.

In other word - this type was here, and removing of this @var produces a phpstan error.

I tried quickly to replace iterable<SplFileInfo|Trace> with iterable<T> in the stack - but that produces too many errors that need to be analyzed separately. Something wrong with types across those classes.

theofidry · 2024-03-18T12:28:28Z

src/TestFramework/PhpUnit/Adapter/PestAdapterFactory.php

@@ -70,7 +72,8 @@ public static function create(
        array $sourceDirectories,
        bool $skipCoverage,
        bool $executeOnlyCoveringTestCases = false,
-        array $filteredSourceFilesToMutate = []


unrelated, but we could update the CS fixer to append the trailing commas all the time to avoid this sort of diff

agree, extracted to #1920

src/TestFramework/PhpUnit/Adapter/PestAdapterFactory.php

theofidry · 2024-03-18T12:30:15Z

src/TestFramework/PhpUnit/CommandLine/ArgumentsAndOptionsBuilder.php

@@ -123,6 +141,34 @@ public function buildForMutant(string $configPath, string $extraOptions, array $
        return $options;
    }

+    private function mapSourceClassToTestClass(SplFileInfo $sourceFile): string


could be static

theofidry · 2024-03-18T12:30:31Z

src/TestFramework/PhpUnit/CommandLine/ArgumentsAndOptionsBuilder.php

+    /**
+     * @return list<string>
+     */
+    private function prepareArgumentsAndOptions(string $configPath, string $extraOptions): array


could be static

it's not a common practice in Infection's codebase, so I decided to leave it as is.

If we decide to use static methods every time there is no $this in it - let's discuss it and probably integrate a fixer? But I wonder what's the benefit and whether it worth doing it.

tests/phpunit/Configuration/ConfigurationFactoryTest.php

# Conflicts: # src/Command/RunCommand.php # src/Configuration/Configuration.php # src/Configuration/ConfigurationFactory.php # src/Container.php # src/TestFramework/Factory.php # src/TestFramework/PhpUnit/Adapter/PestAdapterFactory.php # src/TestFramework/PhpUnit/Adapter/PhpUnitAdapterFactory.php # src/TestFramework/PhpUnit/CommandLine/ArgumentsAndOptionsBuilder.php # tests/benchmark/Tracing/provide-traces-closure.php # tests/phpunit/Configuration/ConfigurationAssertions.php # tests/phpunit/Configuration/ConfigurationFactoryTest.php # tests/phpunit/Configuration/ConfigurationTest.php # tests/phpunit/ContainerTest.php # tests/phpunit/TestFramework/PhpUnit/CommandLine/ArgumentsAndOptionsBuilderTest.php

Related to infection/infection#1919

maks-rafalko · 2024-03-21T00:06:44Z

Wanted to share:

PHP-CS-Fixer/PHP-CS-Fixer#7874 (comment)

Run only those tests that cover modified (or filtered) files to reduc…

3ae06c1

…e the time for Initial Tests Run

maks-rafalko added Performance Feature labels Mar 17, 2024

maks-rafalko mentioned this pull request Mar 17, 2024

Reduce coverage runs when using forceCoversAnnotation="true" + --diff-git-filter? #1451

Closed

maks-rafalko force-pushed the feature/1451-reduce-initial-tests-run-time branch from 0f1547d to b8256b5 Compare March 17, 2024 18:15

maks-rafalko commented Mar 17, 2024

View reviewed changes

maks-rafalko force-pushed the feature/1451-reduce-initial-tests-run-time branch 3 times, most recently from 6138a30 to ddc9d72 Compare March 17, 2024 20:12

Introduce --map-source-class-to-test option to provide a way to ena…

e675584

…ble "Initial Tests Run" test files filtering Implements #1451

maks-rafalko force-pushed the feature/1451-reduce-initial-tests-run-time branch from ddc9d72 to e675584 Compare March 17, 2024 20:22

theofidry self-assigned this Mar 17, 2024

theofidry approved these changes Mar 17, 2024

View reviewed changes

maks-rafalko added 2 commits March 17, 2024 22:03

Rename methods for better clarity

3a272af

Apply small cs fix

e4a81b6

sidz approved these changes Mar 18, 2024

View reviewed changes

theofidry approved these changes Mar 18, 2024

View reviewed changes

CS fixes, more readable in_array() check

609c8eb

maks-rafalko mentioned this pull request Mar 18, 2024

Upgrade php-cs-fixer and add more rules #1920

Closed

Wirone mentioned this pull request Mar 19, 2024

test: Introduce Infection for mutation tests PHP-CS-Fixer/PHP-CS-Fixer#7874

Merged

maks-rafalko added 2 commits March 19, 2024 22:53

Merge branch 'master' into feature/1451-reduce-initial-tests-run-time

4bf6cf8

maks-rafalko added a commit to infection/site that referenced this pull request Mar 20, 2024

Document new --map-source-class-to-test option

3f85437

Related to infection/infection#1919

maks-rafalko mentioned this pull request Mar 20, 2024

Document new --map-source-class-to-test option infection/site#259

Merged

maks-rafalko added a commit to infection/site that referenced this pull request Mar 20, 2024

Document new --map-source-class-to-test option

8a3864e

Related to infection/infection#1919

maks-rafalko merged commit 6d55979 into master Mar 20, 2024
55 checks passed

maks-rafalko deleted the feature/1451-reduce-initial-tests-run-time branch March 20, 2024 07:48

stloyd mentioned this pull request May 10, 2024

Adjust Inflection to run only on changed files flow-php/flow#1073

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce "Initial Tests Run" stage time by running only covering test files #1919

Reduce "Initial Tests Run" stage time by running only covering test files #1919

maks-rafalko commented Mar 17, 2024 •

edited

maks-rafalko Mar 17, 2024

sidz Mar 18, 2024

maks-rafalko Mar 18, 2024 •

edited

theofidry Mar 18, 2024

theofidry Mar 18, 2024

maks-rafalko Mar 18, 2024

theofidry Mar 18, 2024

maks-rafalko Mar 18, 2024

theofidry Mar 18, 2024

theofidry Mar 18, 2024

maks-rafalko Mar 18, 2024

maks-rafalko commented Mar 21, 2024

	public static function getTestClassName(string $sourceClassName): string
	{
	if (strpos($sourceClassName, 'Infection\\Tests') !== false) {
	return $sourceClassName . 'Test';
	}

	return preg_replace('/Infection/', 'Infection\\Tests', $sourceClassName, 1) . 'Test';
	}

Reduce "Initial Tests Run" stage time by running only covering test files #1919

Reduce "Initial Tests Run" stage time by running only covering test files #1919

Conversation

maks-rafalko commented Mar 17, 2024 • edited

Idea explanation

Things to note

Example with PHP-CS-Fixer

Technical comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maks-rafalko Mar 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maks-rafalko commented Mar 21, 2024

maks-rafalko commented Mar 17, 2024 •

edited

maks-rafalko Mar 18, 2024 •

edited