Tests Missed in Execution When Another Test Extends From It

[elblinkin]

If you have a test 'FooTest' which is a concrete test class, and then you extend that concrete test class with test 'BarTest', 'FooTest' will not be executed as it's class was loaded and cached in PHP_Util_Class::$buffer before the test for that file can be added to the test suite.

[elblinkin]

This will also happen if a 'BarTest' simply references a 'FooTest'. So say that I give 'FooTest' a bunch of static methods, and then reference them as dataProvider-s in 'BarTest', same affect.

[edorian]

This is only reproducible when using autoloading or requires for the phpunit test classes. It took me a while to figure that out as the idea seems kind of strange to me to autoload files that phpunit already includes.

So the things that work:

---

Using:

BarTest.php

<?php

class BarTest extends FooTest {

    public function testBar() {
        echo "bar";
        $this->assertTrue(true);
    }
}

FooTest.php

<?php
class FooTest extends PHPUnit_Framework_TestCase {

    public function testFoo() {
        echo "foo";
        $this->assertTrue(true);
    }
}

produces:

 Fatal error: Class 'FooTest' not found in ...

---

Swapping the classes around gives me the expected:

 phpunit .
PHPUnit 3.6.10 by Sebastian Bergmann.

.bar.foo.bar

Time: 0 seconds, Memory: 5.00Mb

OK (3 tests, 3 assertions)

---

And now what doesn't work

BarTest.php

<?php

require __DIR__ . '/FooTest.php';

class BarTest extends FooTest {

    public function testBar() {
        echo "bar";
        $this->assertTrue(true);
    }
}

FooTest.php

<?php
class FooTest extends PHPUnit_Framework_TestCase {

    public function testFoo() {
        echo "foo";
        $this->assertTrue(true);
    }
}

produces::

 phpunit .
PHPUnit 3.6.10 by Sebastian Bergmann.

.bar.foo

Honestly I'd just say "Don't use autoloading or requires in test cases" but as I know people do it (not just you, I've seen that used a couple of times where people just build autoload maps for /tests/ or use file based inclusion also for the test folders (instead of just the test helpers)) I guess that is something we should take care of.

Writing a regression test for that shouldn't be that hard but any pointers on how/where to fix it would be appreciated.

---

Then again the time might be better spent on #10 that should just get rid of the issue :)

[elblinkin]

Honestly, people should not reference something in one test file in another test file. They should be pulling things out into test helpers that live with the code to be tested, not the test code.

I'm fine if it is deemed not to be fixed because honestly you should not implement tests this way, but I wanted to make sure there was some sort of documentation warning of this scenario so that there are fewer surprises.

[ashnazg]

I think I just stumbled into this bug myself. It seems as though its a loading sequencing issue based on the filenames themselves, as BarExtendsFoo fails (Bar.php loaded before Foo.php), while FooExtendsBar succeeds (Bar.php loaded before Foo.php).

Yes, my test class that are affected do use autoloading, one extending the other, and oddly enough they are named exactly the same... the files and class names are identical, only differentiated by folder locations and namespacing.

In my example, ./StatementTest.php is an abstract parent of ./PDO/StatementTest.php. It looks as though the subfolder name "./PDO" is alphabetically chosen before ./StatementTest.php", and therefore ./PDO/StatementTest.php gets read before ./StatementTest.php gets read. Herein lies the loading sequence issue.

Might it be possible to have the loader first load all files in a given directory before it descends into its subdirectories?

[aik099]

This is still happening. Any plans on fixing this after year of no attention?

[aik099]

Any progress on this?

Because of this issue I can't really use https://github.com/brianium/paratest. For example when running tests in parallel I'm getting only 311 tests executed out of 371.

[sebastianbergmann]

Parallel test execution cannot work (correctly, with all features, ...) unless the work outlined in issue #10 has been done.

[whatthejeff]

@aik099–This issue isn't particularly high on our priority list as far as I know. Unless someone has time to work on a PR, it might be some time before we get around to it.

[aik099]

Parallel test execution cannot work (correctly, with all features, ...) unless the work outlined in issue #10 has been done.

Interesting, that this fact isn't mentioned anywhere on https://github.com/brianium/paratest.

@aik099–This issue isn't particularly high on our priority list as far as I know. Unless someone has time to work on a PR, it might be some time before we get around to it.

I can try sending a PR, but I don't understand yet the root of a problem and it's possible solutions. Is it the fact, that test cases are auto-loaded preventing parent test case classes from test discovery? So adding require_once with path to parent test case class will solve the problem?

Symfony is using http://www.gnu.org/software/parallel/ in their TravisCI configuration:

- ls -d src/Symfony/*/* | parallel --gnu --keep-order 'echo "Running {} tests"; phpunit --exclude-group tty,benchmark {};' || exit 1

This looks interesting, since actual test file discovery now is done by ls command. However I'm worried that PHPUnit won't be able to build a united clover.xml file. Just concatenating clover.xml file from different runs won't do the trick, since same classes (of library) might be covered by multiple tests, that are run in parallel.

P.S.
I've started looking on parallel test execution not because of test suite speed, but rather because it's memory consumption. Right now to do a code coverage report a lot of memory is used (>512MB). I hoped, that splitting tests into several processes the memory usage per process would decrease.

[dracony]

So is there any temporary workaround for this? My test structure mimics my code structure, so there is a lot of overloading going on.
At least is there a flag to perform a breadth-first search instead of a depth-first one? I mean usually extended classes are in subfolders so that would "fix" this for me.

[dracony]

Would accept a PR to this method: https://github.com/sebastianbergmann/php-file-iterator/blob/master/File/Iterator/Facade.php#L67

I'd change it to be breadth-first

[chrisguitarguy]

I looked into this a bit, and the issue comes from right here.

PHPUnit_Util_Fileloader::checkAndLoad calls include_once.

The problem arises with get_declared_classes and the diff after the file in question is loaded. If the file has already been included, the classes that reside in it will already be in the first call to get_declared_classes.

The way around this is to not do the the diff. Just get_declared_classes after the include, and check to see if any of the classes reside in the filename in question.

For what it's worth Doctrine ORM has this same problem and solves it the same way.

The problem, of course, is that it's kind of expensive resource-wise to check every class.

[dracony]

Why not load all of the directory tree and only then do the get_declared_classes() check then? All those classes are going to end up in memory anyway.

Also it seems that this is less of a problem on Windows, since it first would look at A.php and only then into the A/ folder. (Put files before directories of the same name)

[aik099]

So if I have class B extends A and I load that file, then get_declared_classes call would get me A and B or only A? I think when test files are auto-loaded (e.g. via Composer) then including file with B class declaration would trigger loading of file with A class declaration (that PHPUnit won't pickup) and that will create a problem.

I think what will help is from the newly added classes really figure out which ones came from a file, that was included and which ones were auto-loaded (see http://www.php.net/manual/en/reflectionclass.getfilename.php). This way we can manually patch file-to-class array based on auto-loaded classes as well.

[whatthejeff]

@dracony I'm not really a huge fan of inheriting tests like this, but I wouldn't mind seeing this issue fixed. Feel free to send PRs so long as they don't just change the order of loading so that it's favorable for a particularly test suite.

[aik099]

But from the addTestFile method you don't know what other files are out there that needs to be loaded.

[dracony]

K. I'll take a look at this.

[dracony]

I think I have a simple fix for this:
#1327

[dracony]

Alright, I found a slightly better solution. Instead of discarding loaded class names they go into the $untestedClasses array. Then that array is being used to match if the class exists for the particular suite.

[dracony]

Well, the pull request is there, but nobody cares

[whatthejeff]

@dracony I left some feedback for you in #1327.

PS: Please have a little bit of patience. I maintain/contribute to a handful of open source projects (some not on GitHub) and work a more-than-full-time job.

[dracony]

Added tests =)

[dracony]

Seriously guys! Can my fix be merged now? It's been a week since I submitted a working fix WITH TESTS

[whatthejeff]

@dracony Please be patient. Your PR has possible BC and performance implications that I need to investigate before I can merge it. I hope to have time this weekend, but no guarantees.

[webmozart]

I just stumbled into this, we have the same issue in the Symfony test suite.

[aik099]

You got that problem when you try to slice and dice test suite to make only specific tests to run. If run all test suite, then no problem happens.

[webmozart]

For me this problem appears always, as a test will never be loaded if its class was loaded before the first call to get_declared_classes() (called before PHPUnit_Util_Fileloader::checkAndLoad($filename);).

[whatthejeff]

I'll look into this today. Thanks for your patience everyone :)

[whatthejeff]

This should be fixed in the next stable release. Thanks for your patience everyone :)