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

Breakfast: rerun defects first #3147

Closed
wants to merge 1 commit into
base: master
from

Conversation

4 participants
@epdenouden
Contributor

epdenouden commented May 28, 2018

Use case

Rerun defective unit tests first to speed up the red-green-refactor cycle!

Test results and timings are shared between tests runs using a cache. This allows for sorting defective tests to the front of the execution order using an additional sorter. This new sort option is compatible with existing test ordering options.

This project is a followup to my previous work on reordering test execution. The code is available in my Breakfast development branch.

Summary of changes

  • the TestSuiteSorter gains the ability to sort suites and tests by priority of defects and execution time
  • added a ResultCacheExtension to gather result and timing information during test runs
  • added a TestResultCache for sharing of result state between runs
  • added caching of run results using CLI flag --cache-result and configuration attribute cacheResult='true'
  • added sorting defects to run as quick as possible with CLI flag --order-by=defects and configuration attribute executionOrder='defects'
  • ability to specifiy the cache filename using CLI flag --cache-result-file and configuration attribute cacheResultFile
  • more extensive testing of sorting scenarios

How to test

  1. To activate storing the results of a run use --cache-result:

    phpunit --cache-result
    
  2. For this example we switch the cache on for every run by adding the attribute cacheResult="true" to the <phpunit> element in phpunit.xml.

    <phpunit cacheResult="true"> <-- no other changes required --></phpunit>
  3. Break TestCaseTest by running it backwards:

    phpunit --order-by=reverse tests/Framework/TestCaseTest.php
    
  4. Look at the cache file. By default this is .phpunit.result.cache in the PHPUnit working directory.

  5. Break TestCaseTest again with the sorting feature turning on:

    phpunit --order-by=defects tests/Framework/TestCaseTest.php
    
  6. The skipped tests are still there but are run immediately now, but still fail. This is easily avoided by asking the sorter to keep dependencies in mind:

    phpunit --order-by=depends,defects tests/Framework/TestCaseTest.php
    

Design considerations

Like the New Order feature the aim is for a robust system with sane defaults that silently does its work and is easily maintained by other developers:

  • Require as little new configuration as possible. Just add cacheResult="true" to your configuration and it silently does its work.
  • Add no extra output as PHPUnit often lives in automated pipelines and dev scripts.
  • Provide robust and maintainable code. This feature also provides a lot of new test coverage for functionality from #3092.
  • Implemented a simplistic cache on purpose. Obviously people might want more complex caches based on users, groups, configuration, git hash, etc etc. Let's see what people need.
@codecov-io

This comment has been minimized.

codecov-io commented May 28, 2018

Codecov Report

Merging #3147 into master will increase coverage by 0.59%.
The diff coverage is 98.24%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master   #3147      +/-   ##
===========================================
+ Coverage      81.5%   82.1%   +0.59%     
- Complexity     3412    3506      +94     
===========================================
  Files           137     140       +3     
  Lines          9014    9218     +204     
===========================================
+ Hits           7347    7568     +221     
+ Misses         1667    1650      -17
Impacted Files Coverage Δ Complexity Δ
src/Util/Configuration.php 97.11% <100%> (+0.68%) 179 <0> (+7) ⬆️
src/Runner/ResultCacheExtension.php 100% <100%> (ø) 13 <13> (?)
src/Util/NullTestResultCache.php 100% <100%> (ø) 4 <4> (?)
src/Runner/TestSuiteSorter.php 100% <100%> (+6.12%) 43 <28> (+14) ⬆️
src/TextUI/Command.php 71.5% <100%> (+3.22%) 204 <7> (+11) ⬆️
src/Util/TestResultCache.php 100% <100%> (ø) 25 <25> (?)
src/Framework/TestResult.php 72.75% <100%> (+0.65%) 163 <1> (+5) ⬆️
src/TextUI/TestRunner.php 63.96% <84.61%> (+1.56%) 274 <0> (+15) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1bde207...7dc6729. Read the comment docs.

@epdenouden epdenouden force-pushed the epdenouden:breakfast branch 3 times, most recently from 4f40a19 to 476c1ea May 28, 2018

@sebastianbergmann

This comment has been minimized.

Owner

sebastianbergmann commented May 29, 2018

  • This branch cannot be rebased due to conflicts.
  • Why is the ResultCacheListener needed, or rather why is it needed to be known to the user? As this pull request is about adding functionality to the "core" of PHPUnit, there is no need to expose the usage of an extension point like that. We can simply attach the listener when we need it.
  • I think we should add a CLI switch and configuration option to record test results instead.
  • I would prefer not to use the old, bloated TestListener interface for this but rather the new hook interfaces (introduced in PHPUnit 7.1 with #3002), if possible.

@sebastianbergmann sebastianbergmann added this to the PHPUnit 7.3 milestone May 29, 2018

@epdenouden epdenouden force-pushed the epdenouden:breakfast branch from 0b63afb to 15848af May 29, 2018

@epdenouden

This comment has been minimized.

Contributor

epdenouden commented May 29, 2018

Thanks for the quick review, again!

  • Branch is fixed. The culprit was a backported TestSuiteSorterTest. Doing a git rebase instead of merge even suggested just removing a conflicting commit altogether.
  • I extended the public TestListener based on the example in the manual. I would also prefer to keep this internal. Will refactor into an extension that can be switched on with a command line flag, also makes testing cleaner.
/**
* @var string
*/
public const DEFAULT_RESULT_CACHE_FILENAME = 'result_cache.json';

This comment has been minimized.

@keradus

keradus May 29, 2018

Contributor

if I have my project, eg ~/php-cs-fixer folder, and I would run phpunit with cache enabled, it would create ~/php-cs-fixer/result_cache.json ? if so, please let us make it phpunit.cache.json or sth like this

This comment has been minimized.

@epdenouden

epdenouden May 30, 2018

Contributor

I have no opinion on the filename of the default value. As a developer I want the following:

  1. Simple usage that just works in a basic code, test, repeat cycle. A file in the working directory works well.
  2. PHPUnit is part of automation: use the environment variable to do your devops and have it store the cache(s) for users/configs/build-unique-ID/git-hashes/etc however en whereever you want.

The default implementation of TestResultCache is there so the simplest use cases work out-of-the-box. The current cache is just a simple MVP. I would prefer NOT to use an external dependency like SQLite like PHPUnit Clever and Smart did, but I am starting to see why they went that way.

This comment has been minimized.

@keradus

keradus May 30, 2018

Contributor

well, here I'm even not talking about using SQLite or anything like that. (while Indeed, i can see value of it - yet in caching of PHP CS Fixer we don't use it as well - too fancy for our simple purpose).

what I'm saying here @epdenouden is that if I will run phpunit in my project, using cache and out-of-the-box approach (so I don't configure anything extra), phpunit will leave result_cache.json file there. if phpunit will leave sth, it shall indicate it's his leftover, putting it to the name part.
like

-result_cache
+phpunit_cache

This comment has been minimized.

@epdenouden

epdenouden May 30, 2018

Contributor

@keradus quick reply: that was a good suggestion, thx! Have pushed a change for that.

I mixed two related topics regarding the caching, making my comment confusion. Adding in persistence cleanly has been more of a chore than I expected. Will spend more time tonight on a cleaner approach.

This comment has been minimized.

@keradus

keradus May 30, 2018

Contributor

new name looks shiny, thanks for the change!
then, I will try to review changes tomorrow ;)

return;
}
$this->defects = $cacheData['defects'];

This comment has been minimized.

@keradus

keradus May 29, 2018

Contributor

we have no clue that decoded data contain such key...
and even if - what is the content of it.

it's optimistic approach here.

cache file could be treated as-is, one is not supposed to modify it manually.
maybe serialize and then unserialize with unserialize(..., ['allowed_classes' => CacheData::class]) ?

This comment has been minimized.

@epdenouden

epdenouden May 30, 2018

Contributor

Yes, the earliest versions of Breakfast were just that: patches to the code that quickly (un)serialized a results file to store the state between runs. I'll try some other options.

private function formatJSONOutputStruct(): string
{
return \json_encode([

This comment has been minimized.

@keradus

keradus May 29, 2018

Contributor

json_encode can fail as well (eg on non utf-8 input)

/**
* @var array
*/
private $times = [];

This comment has been minimized.

@keradus

keradus May 29, 2018

Contributor

what is the content of those collections? array of.... ints? is key meaningful or numeric?
how single defect looks like ?

This comment has been minimized.

@epdenouden

epdenouden May 30, 2018

Contributor

Thanks, all good points regarding the results cache. Started refactoring that part to make it more robust and cleaner.

phpunit.xsd Outdated
@@ -227,6 +233,7 @@
<xs:attribute name="printerClass" type="xs:string" default="PHPUnit\TextUI\ResultPrinter"/>
<xs:attribute name="printerFile" type="xs:anyURI"/>
<xs:attribute name="processIsolation" type="xs:boolean" default="false"/>
<xs:attribute name="stopOnDefect" type="xs:boolean" default="false"/>

This comment has been minimized.

@keradus

keradus May 29, 2018

Contributor

would be nice to document somewhere what is a defect

This comment has been minimized.

@epdenouden

epdenouden May 29, 2018

Contributor

Thank you. I have added a description to the help message.

private function updateRunState(\PHPUnit\Framework\Test $test, string $state): void
{
$testName = $this->fullTestName($test);

This comment has been minimized.

@keradus

keradus May 29, 2018

Contributor

too many spaces

This comment has been minimized.

@epdenouden

epdenouden May 29, 2018

Contributor

fixed

];
/**
* @var array

This comment has been minimized.

@keradus

keradus May 29, 2018

Contributor

array of ?

This comment has been minimized.

@epdenouden

epdenouden May 29, 2018

Contributor

Added description

/**
* @var array
*/
private $defectSortWeight = [

This comment has been minimized.

@keradus

keradus May 29, 2018

Contributor

const

This comment has been minimized.

@epdenouden

epdenouden May 29, 2018

Contributor

Fixed, was a precalculated array before.

}
}
private function sort(TestSuite $suite, int $order, bool $resolveDependencies): void
public function setCache(TestResultCache $cache): void

This comment has been minimized.

@keradus

keradus May 29, 2018

Contributor

I believe this property shall not be exposed

This comment has been minimized.

@epdenouden

epdenouden May 29, 2018

Contributor

You're right, the method is only used in the test. Will clean it up :)

use PHPUnit\Framework\TestCase;
use PHPUnit\Framework\TestSuite;
class TestSuiteSorterEmptyTestCaseTest extends TestCase

This comment has been minimized.

@keradus

keradus May 29, 2018

Contributor

there is no TestSuiteSorterEmptyTestCase class, how we can have test for it ?

test here shall be part of TestSuiteSorterTest test class, it's one concrete, edge scenario to be tested, not completely other class

This comment has been minimized.

@epdenouden

epdenouden May 29, 2018

Contributor

Refactored the fixtures in TestSuiteSorterTest and added this test in. Cleaner, thanks for the comment!

It started off as a quick test where I didn't want the fixtures from TestSuiteSorterTest. The name is indeed confusing, it was supposed to mean "TestSuiteSorter (tested with a) EmptyTestCase".

@epdenouden epdenouden force-pushed the epdenouden:breakfast branch 2 times, most recently from 59d043a to b46e416 May 29, 2018

$sorter = new TestSuiteSorter();
$sorter->setCache($cache);
$_sorter = new \ReflectionClass($sorter);

This comment has been minimized.

@keradus

keradus May 30, 2018

Contributor

_? leading _?

what about $sorterReflection ?

also, having a need to access class' internals in test indicate bad class structure. maybe Cache shall be injected to Sorter via constructor ?

This comment has been minimized.

@epdenouden

epdenouden May 30, 2018

Contributor

The $_var as a temporary version of $var as I quickly needed one, like the $_suite in TestRunner. Will pick something else for now, then look at the use of Reflection.

I can imagine $sorter = new TestSuiteSorter($cache) but to me that feels like juggling the hot potato of persisting the cache between runs. When added to the constructor it becomes the problem of the TestRunner which creates the TestSuiteSorter.

Thank you for your detailed comments! I will spend more time experimenting with the code and structure later today.

@@ -51,7 +51,7 @@
];
/**
* @var array
* @var array[string]int Associative array of (string => DEFECT_SORT_WEIGHT) elements

This comment has been minimized.

@keradus

keradus May 30, 2018

Contributor

array[string]int is not proper type according to phpdoc
array<string, int>

This comment has been minimized.

@epdenouden

epdenouden May 30, 2018

Contributor

Thanks! Where did you find this syntax? I saw this recommended a few times, but have found no definitive answer in the manual https://docs.phpdoc.org/guides/types.html

I found some discussions regarding proposals but most of them seem to have gone to sleep, for example: phpDocumentor/phpDocumentor2#650

This comment has been minimized.

@keradus

keradus May 30, 2018

Contributor

psr-5 always-proposal, sadly. but it's widely suported by IDEs and SCAs

@epdenouden epdenouden force-pushed the epdenouden:breakfast branch 3 times, most recently from 0139690 to cbeb934 May 30, 2018

return;
}
$this->loadFromJSON($json);
$cache = \unserialize($cacheData);

This comment has been minimized.

@keradus

keradus May 30, 2018

Contributor

please provide 2nd argument with allowed class deserialization.
now, you are exposing injection point for security hole (for PHPUnit it's not big deal, but still...)

This comment has been minimized.

@epdenouden

epdenouden May 30, 2018

Contributor

Yes thx :) had added that in the meantime. Since you're here, is it very picky about namespacing?

This comment has been minimized.

@epdenouden

epdenouden May 30, 2018

Contributor

Done! :)

This comment has been minimized.

@keradus

keradus May 30, 2018

Contributor

self::class takes FQCN into consideration, so all good ;)

public function testCacheFilenameViaEnv()
{
$_ENV['PHPUNIT_RESULT_CACHE'] = '/some/cache';
$cache = new TestResultCache;
$this->assertEquals('/some/cache', $cache->getResultCacheFilename());
unset($_ENV['PHPUNIT_RESULT_CACHE']);

This comment has been minimized.

@keradus

keradus May 30, 2018

Contributor

if assertEquals would fail,
this line would never be executed

private $runState = [];
/**
* @var TestResultCache

This comment has been minimized.

@keradus

keradus May 30, 2018

Contributor
new ResultCacheListener()

boom ! this property is null, not TestResultCache ;)

This comment has been minimized.

@epdenouden

epdenouden May 30, 2018

Contributor

added some phpdoc ...which I then even had to sort 🤓

@@ -1133,6 +1160,7 @@ protected function showHelp(): void
--include-path <path(s)> Prepend PHP's include_path with given path(s)
-d key[=value] Sets a php.ini value
--generate-configuration Generate configuration file with suggested settings
--cache-result-file==<FILE> Specify result cache path and filename

This comment has been minimized.

@sebastianbergmann

sebastianbergmann Jun 12, 2018

Owner

What about "Specify path to result cache file"?

This comment has been minimized.

@epdenouden

epdenouden Jun 12, 2018

Contributor

It contains a the full $path . $filename not just a path. The filename itself doesn't have to be .phpunit.result.cache

$arguments['timeoutForLargeTests'] = $arguments['timeoutForLargeTests'] ?? 60;
$arguments['timeoutForMediumTests'] = $arguments['timeoutForMediumTests'] ?? 10;
$arguments['timeoutForSmallTests'] = $arguments['timeoutForSmallTests'] ?? 1;
$arguments['verbose'] = $arguments['verbose'] ?? false;

This comment has been minimized.

@sebastianbergmann

sebastianbergmann Jun 12, 2018

Owner

I do not understand this whitespace change.

This comment has been minimized.

@epdenouden

epdenouden Jun 12, 2018

Contributor

Me neither, to be honest. I have just removed all whitespace between ] and = and had it reformatted. Looks like the assignment alignment doesn't remove excess whitespace once it has been introduced. Will commit a WS fix

@epdenouden

This comment has been minimized.

Contributor

epdenouden commented Jun 13, 2018

@keradus @sebastianbergmann In 11360b3 I have added a suggestion for what a cleaner --order-by CLI flag could look like. Having tried it out yesterday evening while working it feels easy to use. Much better than the current clutter of --defects-first-order, --resolve-dependencies and --<sort>-order.

image

Notes:

  • I have kept the existing flags introduced in 7.2 to not break current installations
  • TextUI\Command is not easy to test without doing functional tests or reflection
  • maybe rename flag, finalize --help message
--defects-first-order Run previously unsuccessful tests first
--random-order Run tests in random order
--order-by=<order> Run tests in 'default', 'random' or 'reverse' order
--order-by=defects Run previously failed tests first

This comment has been minimized.

@keradus

keradus Jun 13, 2018

Contributor

not sure about this docs here. it's single option, it shall have single doc entry, in which we explain what is defects

This comment has been minimized.

@keradus

keradus Jun 13, 2018

Contributor

also, looks like depends is missing

This comment has been minimized.

@epdenouden

epdenouden Jun 13, 2018

Contributor

Good points, yes. Wanted to give it a quick try using it myself for a while first. If this is something that @sebastianbergmann also likes I will make it production-ready.

This comment has been minimized.

@epdenouden

epdenouden Jun 13, 2018

Contributor

It barely fits on one line now, but everybody reads the manual... right? :)

private function handleOrderByOption(string $value): void
{
foreach (\explode(',', $value) as $order) {

This comment has been minimized.

@keradus

keradus Jun 13, 2018

Contributor

if I would put as value foobar, it shall fail. now, it's ignored

This comment has been minimized.

@epdenouden

epdenouden Jun 13, 2018

Contributor

Yes. I didn't see other --options throw nice error messages yesterday. Will have a look at how to do this nicely. Silent failures are a big no-no here, I have typed defecst waaaaay too often

This comment has been minimized.

@epdenouden

epdenouden Jun 13, 2018

Contributor

Added a simple error message with failure exit code.

@epdenouden

This comment has been minimized.

Contributor

epdenouden commented Jun 13, 2018

@keradus The --order-by=foobar will now exit with an error message:

./phpunit --order-by=defects,foobar,depends
PHPUnit 7.3-g0a4a8c258 by Sebastian Bergmann and contributors.

unrecognized order-by option -- foobar

The help message is now:

  --order-by=<order>          Run tests in order: default|reverse|random|defects|depends
@@ -1347,6 +1346,8 @@ private function handleOrderByOption(string $value): void
$this->arguments['resolveDependencies'] = true;
break;
default:
$this->exitWithErrorMessage("unrecognized order-by option -- $order");

This comment has been minimized.

@keradus

keradus Jun 13, 2018

Contributor

why double -- ?

This comment has been minimized.

@epdenouden

epdenouden Jun 13, 2018

Contributor

Fixed. Was a leftover from where I found this in GetOpt.

phpunit.xsd Outdated
@@ -171,6 +171,12 @@
<xs:enumeration value="random"/>
</xs:restriction>
</xs:simpleType>
<xs:simpleType name="executionOrderDefectsType">

This comment has been minimized.

@keradus

keradus Jun 13, 2018

Contributor

this shall be removed now, and executionOrderType shall be extended to contain defects and depends

This comment has been minimized.

@epdenouden

epdenouden Jun 13, 2018

Contributor

Removed. I have added a list of the common scenarios that make sense. Couldn't find a nice way to define the list in the XSD to help the tooling with validation and autocomplete.

@@ -1011,6 +1016,14 @@ public function stopOnSkipped(bool $flag): void
$this->stopOnSkipped = $flag;
}
/**
* Enables or disables the stopping for any defect: skipped, error, failure, warning, incomplete or risky.

This comment has been minimized.

@keradus

keradus Jun 13, 2018

Contributor

I believe this description is not up to date now

This comment has been minimized.

@epdenouden

epdenouden Jun 13, 2018

Contributor

Fixed

'testFour' => ['state' => BaseTestRunner::STATUS_FAILURE, 'time' => 1],
'testFive' => ['state' => BaseTestRunner::STATUS_FAILURE, 'time' => 1],
],
['testFive', 'testOne', 'testTwo', 'testThree', 'testFour']],

This comment has been minimized.

@keradus

keradus Jun 18, 2018

Contributor

👎
testFour and testFive failed, so I want to have them as soon as possible, with respect to their dependencies.

I would expect that order, instead:
['testFive', 'testThree', 'testFour', 'testOne', 'testTwo']
or
['testThree', 'testFour', 'testFive', 'testOne', 'testTwo']

as in the end, we claimed to run defects first, not last (vide testFour)

This comment has been minimized.

@epdenouden

epdenouden Jun 18, 2018

Contributor
  • testThree has dependencies on testOne and testTwo
  • testFour has a dependency on testThree

what happens:

  1. after sorting for defects the order is: 4 (failed), 5 (failed), 1, 2, 3
  2. running it like this would skip 4 because it needs to run 3 first
  3. same story for 3: it needs both 1 and 2
  4. the dependency resolver fixes this quietly for us: 5 can stay up front (=defects FIRST) but 4 goes behind 3 which goes behind 1,2 (=defects ASAP ;-)

This comment has been minimized.

@keradus

keradus Jun 18, 2018

Contributor

ha, ok, your first 2 points were not part of description (while dependencies of testThree for TestFour was)

then, all good 👍

This comment has been minimized.

@epdenouden

epdenouden Jun 19, 2018

Contributor

Good but not good enough :) I will document the use of MultiDependencyTest in the dataproviders to avoid some of the confusion.

$arguments['processIsolation'] = $arguments['processIsolation'] ?? false;
$arguments['processUncoveredFilesFromWhitelist'] = $arguments['processUncoveredFilesFromWhitelist'] ?? false;
$arguments['randomOrderSeed'] = $arguments['randomOrderSeed'] ?? \time();
$arguments['registerMockObjectsFromTestArgumentsRecursively']= $arguments['registerMockObjectsFromTestArgumentsRecursively'] ?? false;

This comment has been minimized.

@keradus

keradus Jun 19, 2018

Contributor

a space is missing before =
after solving that, alignment will be back to original one again

This comment has been minimized.

@epdenouden

epdenouden Jun 19, 2018

Contributor

I tried something like that in the past, let's see if your suggestion works nicer.

@keradus
Contributor

keradus left a comment

looks damn good piece of work here !

@epdenouden epdenouden force-pushed the epdenouden:breakfast branch from d13647d to 6b2f472 Jun 19, 2018

@epdenouden

This comment has been minimized.

Contributor

epdenouden commented Jun 21, 2018

@sebastianbergmann Hi Sebastian! @keradus and I think this pull request is done. For new ideas and improvements I will open new PRs in the near future. :)

@epdenouden epdenouden force-pushed the epdenouden:breakfast branch from 16e0235 to 5b16423 Jun 21, 2018

@keradus
Contributor

keradus left a comment

double checked after squash, looks same as my local copy before squashing

@epdenouden epdenouden force-pushed the epdenouden:breakfast branch 2 times, most recently from 771abc2 to 5b16423 Jun 22, 2018

@epdenouden epdenouden force-pushed the epdenouden:breakfast branch from 5b16423 to 7dc6729 Jun 22, 2018

@sebastianbergmann

This comment has been minimized.

Owner

sebastianbergmann commented Jun 24, 2018

Thanks!

@epdenouden

This comment has been minimized.

Contributor

epdenouden commented Jun 24, 2018

🎁 thanks for the merge! :)

@keradus

This comment has been minimized.

Contributor

keradus commented Jun 24, 2018

🎆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment