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

Add functionality to test dependencies by changing test running order #3092

Closed
wants to merge 86 commits into
base: test-reorder
from

Conversation

Projects
None yet
4 participants
@epdenouden
Copy link
Collaborator

epdenouden commented Apr 12, 2018

Note: updated documentation
There is a wiki page with more documentation with examples while the official docs are updated.

Inspiration

Are your unit tests truly independent units?

In the past I have often been bitten by test suites that do not test what they claim to test. A common reason is hidden dependencies in the runtime context. Worst case this has caused applications to fail when deployed in production environments.

Changing the order of unit tests turns out to be a simple and effective way of bringing these assumptions out in the open.

Thanks to @Crazybus and @geogdog for providing real-world scenarios and feedback during development.

Summary of functionality

The test ordering functionality can be configured using command line parameters:

  • --reverse-order run tests in reverse order
  • --random-order run tests in random order
  • --random-order-seed=<number> fix the random order, allowing random runs to be reproduced
  • --ignore-dependencies disable the bundled dependency resolver

Design decisions

Low impact, high performance

The feature will only wake up when there is work to do. The iterative dependency resolver is designed to do as little work as possible, using few iterations and minimal memory. It uses @depends annotations to keep as many dependent tests as possible in the required order.

No unnecessary extra output is generated. Only the randomizer outputs the random seed using the default results printer.

Clean abstraction

The reordering is built into the main test runner and only changes the order of the tests of each test suite. Other parts of PHPUnit are not affected, with the exception of a bit of configuration handling.

Changes to the built-in testsuite

  • added a fourth MultiDependencyTest and updated existing tests to match
  • added new TextUI tests for --reverse-order, --random-order and --ignore-dependencies
  • added @depends for testGlobalsBackupPre() to PHPUnit's own testsuite

Future work

  • update the main PHPUnit documentation to reflect the changes in (optional) order and dependency handling
  • add configuration options for phpunit.xml in addition to the command line flags
  • add options and/or annotations for finer grained reordering config (e.g. "never reorder this suite")
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 12, 2018

Codecov Report

Merging #3092 into test-reorder will not change coverage.
The diff coverage is 96.57%.

Impacted file tree graph

@@               Coverage Diff               @@
##             test-reorder    #3092   +/-   ##
===============================================
  Coverage           81.22%   81.22%           
  Complexity           2947     2947           
===============================================
  Files                 110      110           
  Lines                7707     7707           
===============================================
  Hits                 6260     6260           
  Misses               1447     1447
Impacted Files Coverage Δ Complexity Δ
src/Framework/TestSuiteIterator.php 100% <ø> (ø) 9 <0> (ø) ⬇️
src/Util/TestDox/NamePrettifier.php 97.61% <100%> (ø) 22 <0> (ø) ⬇️
src/Util/TestDox/CliTestDoxPrinter.php 88.09% <100%> (ø) 28 <0> (ø) ⬇️
src/Framework/DataProviderTestSuite.php 100% <100%> (ø) 3 <2> (ø) ⬇️
src/TextUI/Command.php 68.02% <100%> (ø) 192 <0> (ø) ⬇️
src/Framework/TestCase.php 67.86% <100%> (ø) 307 <1> (ø) ⬇️
src/TextUI/TestRunner.php 62.47% <100%> (ø) 255 <0> (ø) ⬇️
src/Util/TestDox/RunningTestResult.php 94.64% <94.64%> (ø) 24 <24> (ø) ⬇️
src/Runner/TestSuiteSorter.php 95.74% <95.74%> (ø) 23 <23> (ø) ⬇️
src/Util/TestDox/BootstrappingTestResult.php 96.29% <96.29%> (ø) 8 <8> (ø) ⬇️
... and 6 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 b2a1231...248009b. Read the comment docs.

$this->assertFalse($_ENV['foo']);
$this->assertEquals(1, \getenv('foo'));
$this->assertEquals($fooValue, $_ENV['foo']);
// $this->assertEquals(false, \getenv('foo'));

This comment has been minimized.

@sebastianbergmann

sebastianbergmann Apr 12, 2018

Owner

Is this commented out intentionally?

This comment has been minimized.

@epdenouden

epdenouden Apr 12, 2018

Author Collaborator

Thanks for catching that one. I did not yet know what to do with this one, $_ENV and getenv()/putenv() are not really equivalent. Setting $_ENV does not change the UNIX environment variables, but putenv() does.

Uncommenting this one makes the test fail. Do you have a preference?

/**
* @param string $order
*/
public function setTestRunningOrder($order): void

This comment has been minimized.

@sebastianbergmann

sebastianbergmann Apr 12, 2018

Owner

Please use scalar type declarations when possible (like here). Also please do not use @param annotations etc. when they provide no additional information.

This comment has been minimized.

@epdenouden

epdenouden Apr 12, 2018

Author Collaborator

Fixed

/**
* Reorder Tests within a TestCase in such a way as to resolve as many dependencies as possible.
* The algorithm will leave the tests in original running order when it can.

This comment has been minimized.

@sebastianbergmann

sebastianbergmann Apr 12, 2018

Owner

What about cross-class dependencies? They are possible, though I rarely see them "in the wild".

This comment has been minimized.

@epdenouden

epdenouden Apr 12, 2018

Author Collaborator

I am aware of those, yes, although I have not encountered them. They are the reason I made this change: epdenouden@36697f0

Currently the business logic isn't smart enough yet to also sort the TestSuites based on development. The first work in the form of TestSuite::getDependencies() is there.

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Apr 12, 2018

Thank you, @epdenouden, for working on this. I will look into this more deeply as soon as I can. I hope we can get this into PHPUnit 7.2.

@sebastianbergmann sebastianbergmann added this to the PHPUnit 7.2 milestone Apr 12, 2018

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Apr 12, 2018

@epdenouden I have moved the current state of this to a new branch: https://github.com/sebastianbergmann/phpunit/tree/test-reorder.

Please rebase this pull request onto that branch and target that branch for future changes. Thanks!

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Apr 13, 2018

I am wondering whether it would make sense to move the reorder logic from TestSuiteIterator into separate classes, one for each type of reorder (random, dependencies, ...).

Pseudo Code:

<?php
class DependencyResolver extends ArrayIterator
{
    public function __construct(TestSuiteIterator $tests)
    {
        $tests = iterator_to_array($tests);

        // reorder $tests

        parent::__construct($tests);
    }
}
@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Apr 13, 2018

I am wondering whether it would make sense to add an extension hook for test reordering. This would make @padraic and everyone interested in #2660 happy.

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Apr 13, 2018

@lstrojny This is probably interesting to you, too, considering "clever and smart".

@epdenouden

This comment has been minimized.

Copy link
Collaborator Author

epdenouden commented Apr 13, 2018

@sebastianbergmann Thanks for the quick replies! I will rebase and continue making the basic version clean and stable for 7.2. Good ideas for making this cleaner and more extensible. I am up for doing more coding for that.

Re: the clever and fast logic: this project is actually the groundwork for something that we call the Breakfast extension, it automatically sorts the tests to run skipped, failed, etc first, but keeps their dependencies working. It also measures test and suite timings to make better sorting predictions.

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented Apr 13, 2018

The "breakfast" thing sounds interesting. Lets get this sorted first, then have a look at that. Maybe it can also be integrated into the "core", maybe we can put it into an extension that uses the new, yet-to-be-added hook.

@sebastianbergmann sebastianbergmann force-pushed the sebastianbergmann:test-reorder branch from 0e0f522 to 70c053f May 6, 2018

@epdenouden

This comment has been minimized.

Copy link
Collaborator Author

epdenouden commented May 7, 2018

Not sure what to do about the Travis failing build. The change to PHPStan level 2 in f59852f causes it to fail.

@sebastianbergmann sebastianbergmann force-pushed the sebastianbergmann:test-reorder branch from b6d138d to b2a1231 May 7, 2018

epdenouden added some commits May 7, 2018

Remove unnecessary assignment of unknown property to SplObjectStorage
The ->foo property is not tested for in the TextUI test, only the
existence of a SplObjectStorgage object. This assignment causes PHPStan
to fail now that it is testing with --level=2.
@epdenouden

This comment has been minimized.

Copy link
Collaborator Author

epdenouden commented May 7, 2018

Found the missing piece of PHPStan configuration that went missing while merging a25126a. PHPStan works as before.

@keradus
Copy link
Contributor

keradus left a comment

great work in general!

let me just point out 2 bc breakes for edge-case users

@@ -14,21 +14,18 @@
/**
* Iterator for test suites.
*/
class TestSuiteIterator implements RecursiveIterator
final class TestSuiteIterator implements RecursiveIterator

This comment has been minimized.

@keradus

keradus May 8, 2018

Contributor

btw, this is a BC breaker for all of us that were using this class. (class is not marked as @internal)

This comment has been minimized.

@epdenouden

epdenouden May 8, 2018

Author Collaborator

In the end I didn't need to change many of the existing internals as the code moved to TestSuiteSorter. Extensions support is coming, I tried that first :)

@@ -9,147 +9,19 @@
*/
namespace PHPUnit\Util\TestDox;
final class TestResult

This comment has been minimized.

@keradus

keradus May 8, 2018

Contributor

btw, this is a BC breaker for all of us that were using this class. (class is not marked as @internal)
making it non-class (->interface) is even bigger change

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented May 8, 2018

@keradus PHPUnit still lacks a clear statement of what is considered part of its public API. I hope to have this in place "soon", at the latest in time for PHPUnit 8.

Off the top of my head, I would say that everything internal with the exception of Test, TestCase, TestListener, etc.

@keradus

This comment has been minimized.

Copy link
Contributor

keradus commented May 8, 2018

I know there is no clear statement on that, but especially as it's not documented, all is public and non-internal :(

@keradus

This comment has been minimized.

Copy link
Contributor

keradus commented May 13, 2018

just wondering... can we make random-order a default one please?
or at least, allow to set it via config file

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented May 13, 2018

-1 for default (because of --testdox, for instance)
+0 for configuration

Come to think of it, we should probably disable the dependency resolution by default in PHPUnit 7.2 and turn it on by default in PHPUnit 8.

@keradus

This comment has been minimized.

Copy link
Contributor

keradus commented May 13, 2018

value of this PR is nice, yet requires manual work that one always remember to use that flag.
with config file, it would be way easier

@sebastianbergmann

This comment has been minimized.

Copy link
Owner

sebastianbergmann commented May 13, 2018

@keradus I have opened #3131 for this and will (hopefully) implement it tomorrow.

@keradus

This comment has been minimized.

Copy link
Contributor

keradus commented May 13, 2018

Thanks for accepting the request 👍
no need to treat this one as prio, no rush on it ;)

@epdenouden

This comment has been minimized.

Copy link
Collaborator Author

epdenouden commented May 14, 2018

Indeed thanks for merging it in. I will continue on the followup feature for rerunning failures first now

@epdenouden

This comment has been minimized.

Copy link
Collaborator Author

epdenouden commented May 14, 2018

@keradus having reordering as a default would be fun, but it would spook a lot of people that run phpunit as "any 7.x" version and suddenly things are breaking without them chainging anything. Testing frameworks are important for creating trust that is not so great :)

@keradus

This comment has been minimized.

Copy link
Contributor

keradus commented May 14, 2018

Indeed thanks for merging it in. I will continue on the followup feature for rerunning failures first now

Sounds like https://github.com/fiunchinho/phpunit-randomizer is obsolete, now it's time for
https://github.com/lstrojny/phpunit-clever-and-smart ? :)

having reordering as a default ...

That's why I suggested to have at least config option, thus one don't need to remember about option switch every single time.
On the other hand, if I have tests that are always passing, and then we would detect that assertSame doesnt work for string for some weird reason, I do expect assertSame to be fixed and yes, my tests would start failing. It is not that my test was working and suddenly it stopped. It's that my tests were not able to detect real issue in my application, and after update of testing framework they are. And it's super cool to know that my src has bugs.

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