Follow-up to #2056607: /core/phpunit.xml.dist only finds top-level contrib modules

Plus ça change, plus ça meme bug.

Problem/Motivation

Make a contrib module in ./module, give it PHPUnit tests, type ./vendor/bin/phpunit -c core/, and you're happy.

Put that same contrib module in a subdirectory, such as ./module/folder/my_module/, and its tests vanish from PHPUnit's consciousness.

This is a problem because run-tests.sh finds and runs tests for the module, if you specify --module or --directory or just the module's group. This means the behaviors of these two test runners are out of sync, since phpunit won't find that module.

Proposed resolution

See comment #90 for the thought process: #2499239-90: Use test suite classes to discover different test types under phpunit, allow contrib harmony with run-tests

Since we can't simply specify a configuration using directory and ignore, we have to write our own suite loaders.

Extend \PHPUnit_Framework_TestSuite with suite loaders for our different testing suites. Implement ::suite() which is a factory method used by PHPUnit to allow us to generate the suite.

Suite loaders should use existing test discovery methods in order to more closely harmonize with the behavior of TestDiscovery and run-tests.sh.

Configure phpunit.xml.dist to use these suite loaders for our suites.

How to review this patch:

Install the Examples project. Try to get phpunit to discover the tests for phpunit_example. Like this:

$ cd root/of/drupal
$ drush dl examples
// Examples is downloaded to modules/
$ ./vendor/bin/phpunit -c core/ --list-groups
// See phpunit_example listed.
$ ./vendor/bin/phpunit -c core/ --group phpunit_example
// phpunit runs phpunit_example tests.

Remaining tasks

  • Inject root into the drupal_phpunit_*() functions present in bootstrap.php so that we can be sure they're using the same root directory as the suite loaders.
  • Write tests.

User interface changes

Using the phpunit tool to run tests is now more similar to run-tests.sh when it comes to specifying --group for all modules.

API changes

Comments

Mile23’s picture

Priority: Major » Normal
Status: Closed (fixed) » Active
Mile23’s picture

Status: Active » Needs review
FileSize
657 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,536 pass(es). View

This patch changes the scope of search for the 'unit' test suite.

Being too specific in this search means that contrib is highly constrained in its use of unit tests.

socketwench’s picture

Ugh. This would skip any and all submodules tests, wouldn't it?

webchick’s picture

Issue tags: +Needs tests
Kazanir’s picture

As I understand it, the previous issue's change broke all submodule tests by changing the module PhpUnit include path to a single layer. Instead it should be possible to revert that change but preserve the previous fix by using a **, like so:

  <directory>../modules/**/tests/src/Unit</directory>
  <directory>../sites/*/modules/**/tests/src/Unit</directory>

However, I'm reading the documentation at https://phpunit.de/manual/current/en/appendixes.configuration.html and it looks like we should actually be using the <filter> element (rather than the <testsuite> definition?) to exclude files from code coverage reporting. The relevant piece of the docs appears to be here.

If adding tests for this is hard (and from the IRC discussion it appears to be at least non-trivial) then let's at least fix this in the most right way possible and comment the XML extensively to prevent this in the future.

jhedstrom’s picture

The change to phpunit.xml.dist went in as part of #2232861: Create BrowserTestBase for web-testing on top of Mink, which had the added bonus of accidentally fixing #2203747: Current PHPUnit configuration scans into contrib vendor dirs. PHPUnit whac-a-mole.

Kazanir’s picture

Oh I was confused. I think Mile23 linked the wrong issue by accident; jhedstrom is right and the diff of the testsuite refactoring is here.

Unfortunately, I'm not sure we can solve both of these problems with a static .xml.dist because PHP's glob() function turns out to not support the globstar technique I suggested above. (And it looks like PHPUnit's File_Iterator doesn't do anything other than invoking PHP's glob() to resolve wildcards.)

cilefen’s picture

+++ b/core/phpunit.xml.dist
@@ -15,8 +15,8 @@
+      <directory>../sites</directory>

That is scary.

Kazanir’s picture

Yeah, I dug into PHPUnit's File_Iterator some more and it looks like it's really hard to achieve all of our desires here. I also noticed that the changes in #2232861: Create BrowserTestBase for web-testing on top of Mink also had a lot of other side effects, and they exclude a lot of tests in core modules, e.g.

./modules/views/src/Tests/

Is no longer being picked up. I'm not sure if that's intentional or not...but I suspect not?

Anyway, the problem here is that there is no support for pattern matching in PHPUnit's file iterator. It can match prefixes and suffixes, and it can exclude entire directories, but it doesn't support anything like globstar (the ** match-any-number-of-directories operator) or pattern matching for including and excluding files. So we cannot say, e.g., "include everything in both modules directories but then exclude anything containing the word vendor." This also seems to make it impossible to get nested module tests and support the split between test suites envisioned in the BrowserTestBase issue.

Mile23’s picture

<directory>../sites</directory>
That is scary.

Behold Drupal.

No one uses multisite. The reason we have it is so we can RUN TESTS. :-)

cilefen’s picture

@Mile23 Ehem?

Xano’s picture

I've been thinking about this for a while, and I don't think I've ever seen a project of which the PHPUnit configuration allows for testing the projects dependent's. I've been shipping phpunit.xml.dist with all my Drupal 8 contrib modules. What's against doing that for all of contrib? Doing so means we won't enforce any directory structure limitations on contrib, which is what any solution to this issue would have to do.

alexpott’s picture

One other problem to consider is how does the contrib module get tests on drupal.org.

I was wondering if we could dynamically generate the testsuites somehow and do our own test discovery in the process.

alexpott’s picture

In many ways I think the solution that @Xano suggests is the right way to go.

OTOH maybe we could add contrib tests using a listener. Something akin to the last comment on https://stackoverflow.com/questions/9535078/phpunit-different-bootstrap-...

alexpott’s picture

Spent some time playing with the idea in #14. it doesn't really work because the list of tests is worked before you can hook into it and add more :( And although it is possible to add tests it breaks filtering and limiting the run to a specific test suite.

alexpott’s picture

After spending a couple of hours looking at this if we want automated tests to work on d.o as things stand the best thing will be to only allow PHPUnit tests on the project level for contrib. That way we can keep the functional and unit split. The other solution is to remove the functional and unit split for contrib and do something like...

  <testsuites>
    <testsuite name="core-unit">
      <directory>./tests/Drupal/Tests</directory>
      <directory>./modules/*/tests/src/Unit</directory>
      <!-- Exclude Composer's vendor directory so we don't run tests there. -->
      <exclude>./vendor</exclude>
      <!-- Exclude Drush tests. -->
      <exclude>./drush/tests</exclude>
    </testsuite>
    <testsuite name="core-functional">
      <directory>./tests/Drupal/FunctionalTests</directory>
      <directory>./modules/*/tests/src/Functional</directory>
      <!-- Exclude Composer's vendor directory so we don't run tests there. -->
      <exclude>./vendor</exclude>
      <!-- Exclude Drush tests. -->
      <exclude>./drush/tests</exclude>
    </testsuite>
    <testsuite name="contib">
      <directory>../modules</directory>
      <directory>../sites</directory>
    </testsuite>
  </testsuites>

However test discovery performance will suck as contrib grows because all code will have to checked to see if it is a phpunit class.

Crell’s picture

alexpott: I'm not clear what you're suggesting. Are you suggesting that we make each contrib module have its own phpunit.xml.dist file, and it's on its own, and thus will not get run by phpunit in the site root? (I'd be OK with that.) Is that what you mean by "project level"?

twistor’s picture

I agree with @Xano as well. Each module should ship its own phpunit.xml.dist.

As to how we run the tests on D.O. Why wouldn't cd $module_dir && phpunit work?

alexpott’s picture

By project level I mean we don't support PHPUnit tests in sub modules. But actually even what we have now won;t work if some does modules/contrib/devel and modules/custom/my_custom_module.

So yep I think we should just stop trying to run everything with phpunit.xml.dist and change how run_tests.sh/Simpletest runs PHPUnit.

Crell’s picture

I am far more concerned with /modules/custom/my_custom_module than with /modules/mymodule/submodule, personally.

So yep I think we should just stop trying to run everything with phpunit.xml.dist and change how run_tests.sh/Simpletest runs PHPUnit.

Er. I'm already using phpunit as the runner for all of my PHPUnit-based tests, and encouraging others to do so. That's what incoming PHP devs will be expecting. Why do we want to have a wrapper runner for PHPUnit when it's already got one?

Mile23’s picture

Why do we want to have a wrapper runner for PHPUnit when it's already got one?

Because Drupal, maaaan.

But seriously.

There are two design directions to come from here: 1) Drupal's contrib layout is wrong for PHPUnit. (This is my view.) 2) We should DrupalWTF-ify contrib modules and make our own test runner and so forth. (@Xano in #12, @sun in other issues... Me at other times...)

So.

Consider that we could demand that contrib goes into modules/ (and themes/ and profiles/) if you expect to be found by PHPUnit. And then we'd be done. We could also do this for SimpleTest, but that's a whole other conversation.

The testbot can just always put contrib in those places for D8. (Last I checked, the run-tests.sh uses dist.phpunit.xml as well.)

Call this the 'break sites/' solution.

As I made unclear in #10, the reason we care about sites/ is because of multisite, which I'd say isn't widely used, but which we must keep so we can have SimpleTest. So therefore, toss it from the realm of test discovery while keeping it in the realm of functionality.

Mile23’s picture

FileSize
618 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,395 pass(es). View

And here's a patch for it.

This finds unit tests in modules/examples/phpunit_example/ for instance.

$ ./vendor/bin/phpunit --group phpunit_example
PHPUnit 4.6.4 by Sebastian Bergmann and contributors.

Configuration read from /Users/paulmitchum/projects/drupal8/core/phpunit.xml.dist

................................

Time: 5.91 seconds, Memory: 121.75Mb

OK (32 tests, 35 assertions)
twistor’s picture

An attempt to clarify what I was saying in #18.

Why don't we just remove the ability to run unit tests from Simpletest/run-tests.sh and add another step in testbot that executes phpunit tests for the appropriate project? That way core's phpunit.xml.dist wouldn't have to be concerned with contrib at all. We could even get clever and point to a default phpunit.xml file if the module doesn't have one, so we can provide listeners and a bootstrap file.

While working on Feeds for D8, I've never gone to core/ and executed the tests from there using a --group flag or whatever. I just stay in my module directory and run phpunit.

Crell’s picture

#23 is more what I'm thinking.

cd modules/mydir/somewhere/over/the/rainbow && phpunit

Done. Module devs can do that, and trivially get their tests filtered to just their module. If they have many submodules... well, it's their phpunit.xml.dist file, do what they want.

For testbot, when testing a module, it does the same thing. drush dl cd modules/rainbow && phpunit. kthx

All we lose in this case is "RUN ALL THE TESTS ON ALL THE MODULES ALL AT THE ONCE!" Which, given how long they take to run, I am unlikely to ever want to do anyway. (And once a site is actually built, let's be honest, many core tests don't work anyway due to form alters and theming.)

I think we're over-engineering this question.

Mile23’s picture

#23:

Why don't we just remove the ability to run unit tests from Simpletest/run-tests.sh and add another step in testbot that executes phpunit tests for the appropriate project?

Because that would mean we're removing something useful because we can't figure out configuration. :-)

#21 and #23 both simplify the issue and that's good. Either solution is a change, and either solution will require change to expectation and workflow.

With #21 we don't lose the ability to run everything at once, we're always using the same version of phpunit (cd into your module and unless you type '../../..etc../phpunit' you're using a different executable), we can say phpunit --group my_module, and we don't have the DrupalWTF of requiring you to cd into your module to run tests.

Basically a policy that says 'You have to put your modules in a certain place in order to automatically run unit tests,' is less disruptive and more flexible than 'You can't run unit tests automatically at all.'

The architectural issue has been an ongoing conversation... I just found this one: #2209283: Remove /sites/all in favor of top-level directories

Crell’s picture

and we don't have the DrupalWTF of requiring you to cd into your module to run tests.

I disagree that's a Drupal WTF. I can't say I've ever run all of the tests in my vendor libs in a Composer-based project, ever. All of my own tests, yes, but not all tests of all libs I'm depending on. That seems like the Drupalism.

A policy of "you have to put your modules in X location for tests to work" means "you have to put your modules in X location, period". Unless each module has its own phpunit.xml.dist file, you can't actually run that module's tests at all if you don't. Which means /modules/custom/ is right out, not an option. That feels more problematic to me.

rreiss’s picture

So what is the solution for those who using the modules/custom and modules/contrib structure? is it a bad practice now on Drupal 8? (I guess not..)
What about the submodules structure? how do I make PHPUnit find our custom tests?

As for #18 - I guess that we don't want to run our custom tests in separate from the rest of the tests,
I think that phpunit.xml.dist in each contrib / custom module is a nightmare..

Why not adding the best practices to the core's phpunit.xml.dist file? (e.g. ../modules/*/*/tests/src/Unit)

pfrenssen’s picture

#23 matches what is typically done in the wider PHP world. I'm in favor for that.

What would be the use case of running PHPUnit tests of core + all contributed modules + all custom modules at the same time?

In a typical development workflow all third party code is considered to be "trusted". If you are working on a site that has custom code on top of Drupal core and a bunch of contrib modules you would only run the tests of your own custom developed modules. You trust that the tests of Drupal core + the contrib modules are green.

It's not practical otherwise, because it would imply you have to take responsibility for all test failures in third party code.

Novitsh’s picture

From my point of view, you should only cover your own work with tests. If everybody does this, what a great world it would be :)
I agree on #23 and #28.

However, is this is the outcome, it needs to be documented very well.

Can this be closed?

jhedstrom’s picture

Priority: Normal » Major

I think this is major, since it hides tests, and folks don't have a clear path on how to run those. I'm fine with the approach outlined in #23.

Mixologic’s picture

The testbot needs to run all phpunit tests for a *project* not a module for contrib tests, so, at the very least this needs to be able to run all of the unit tests defined in a module, as well as all of its included submodules.

Also including a reference to adding the ability to have run-tests.sh *not* run phpunit tests so that we can use that for simpletest, and use phpunit's runner for the testbots, as it is, somewhat related.

Mile23’s picture

Even if we used PHPUnit's runner for those tests, it still won't find them because this is a configuration issue with PHPUnit.

Mile23’s picture

Patch in #22 still applies.

Note that we call PHPUnit differently, since vendor is now in the root directory.

$ ./vendor/bin/phpunit -c core/ --group phpunit_example
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.



Time: 8.99 seconds, Memory: 141.25Mb

No tests executed!

$ git apply 2499239_22.patch 

$ ./vendor/bin/phpunit -c core/ --group phpunit_example
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.

................................

Time: 10.8 seconds, Memory: 146.25Mb

OK (32 tests, 35 assertions)
Crell’s picture

Mixologic: If we had a policy of "core's phpunit.xml doesn't touch contrib; for your module, here's a template phpunit.xml file to use but otherwise it's on you to make sure it works properly, like if you're doing submodules or something" would that be workable from an infra standpoint?

Put another way, testbot need only download a package, cd into the package, and run "phpunit". It's the module developer's job to make sure that finds all tests (just as it is for any Composer package developer that's using Travis or Circle CI or whatever).

Side note: We really need to start discouraging multi-module packages. They made sense ten years ago, but no longer. We should actively discourage them now and try to remove support for them in D9.

Mile23’s picture

So again, I'm not sure why it's a horrible idea to have PHPUnit scan all directories where there might be a module, to the extent that we have to plan to disallow submodules in Drupal 9 in 5 years.

Mixologic’s picture

Put another way, testbot need only download a package, cd into the package, and run "phpunit".

Yes, that would be ideal. We just currently have several multi module packages that already have an expectation that their tests will be run, so we either need to communicate to those devs that the current interface (automatically discovering their tests by file scanning) is changing, to be replaced with another method (they explicitly provide the phpunit.xml), or support both for some period of time. From an infra standpoint, we do not want to have to provide an automated testing UI for each individual submodule in a project.

Side note: We really need to start discouraging multi-module packages

- That makes a lot of sense from a "php packages" standpoint, but many of the multimodule packages are mini platforms in and of themselves (commerce/ubercart) Or they provide a base functionality, and a menu of modules that dont work without that base, but are not all necessary for every site (i.e. xml_sitemap). Im not familiar enough with the architecture of d8, and if all the current use cases for "submoduling" can be easly accomplished. Perhaps its in the change records somewhere, but maybe a blog post about "dont do submodules, do x in d8 now that we have autoloading + a service container" or whatever that actually works out to being.

jhedstrom’s picture

So again, I'm not sure why it's a horrible idea to have PHPUnit scan all directories where there might be a module

This, coupled with the idea that a CI environment only has the current module + it's composer.json file, would lead to an old issue wherein vendor PHPUnit tests were being run, and throwing fatal errors (unless vendor were explicitly excluded).

Another issue with

Put another way, testbot need only download a package, cd into the package, and run "phpunit".

is that contrib would not be able to depend on Drupal's UnitTestBase, or any of the new PHPUnit-based test classes (Kernel and Browser), unless Drupal was declared as a dev dependency in each and every composer.json.

Mile23’s picture

That makes a lot of sense from a "php packages" standpoint, but many of the multimodule packages are mini platforms in and of themselves (commerce/ubercart) Or they provide a base functionality, and a menu of modules that dont work without that base, but are not all necessary for every site (i.e. xml_sitemap).

So we have two sets of user needs:

1) I want to run PHPUnit in one place from the command line for all my testing needs. This is nice-to-have.

2) The testbot wants to be able to just execute one script and be done, for both core and contrib, running all submodules' tests. This isn't essential for core, but it's pretty close to essential for contrib, and it also means not developing a ton of special cases within the testbot runner. That is, if you're making a D8 contrib module you assume it runs under the D8 environment, so that's what the testbot gives you.

There are a few ways to satisfy the testbot:

1) Say no. You can't run submodule tests. ::crosses arms::

2) Add scanning for all submodules to dist.phpunit.xml.

3) Have another phpunit.xml file somewhere, named something like contrib.phpunit.xml. This then becomes the only special case the testbot has to deal with.

Option 3 also has the side effect of letting me say ./vendor/bin/phpunit -c contrib.phpunit.xml --group myModule.

The downside of option 3 is that we then have to maintain parity between it and dist.phpunit.xml, and work some magic on simpletest so it knows about it. No free lunch. Sigh.

Mixologic’s picture

One other caveat: I dont think that the testbot will necessarily get away from needing to run phpunit tests via run-tests.sh. The phpunit command line runner does *not* handle fatal errors in tests gracefully. The wrapper of run-tests.sh around php-unit's testrunner allows us to execute each test in an isolated php process, and more importantly, handles the case where the test has some sort of syntax error or causes a core dump in php. If you run phpunit with --process-isolation, it runs all the tests, but the failed tests produce no output other than the php fatal error/segfault to stderr. Essentially those tests act like they've been skipped - there is no test artifacts like xml. In order to track which tests fail like that so we can provide a complete list of passed and failed tests (regardless of reason), we have to have a wrapper around phpunit that either looks at the stderr/stdout of the phpunit runner, or creates a fail record that is replaced with whatever the real output of phpunit is (like we're doing now).

So, keep that in mind when we talk about testbot requirements. Maybe this issue doesnt affect testbots at all.

tim.plunkett’s picture

Title: /core/phpunit.xml.dist only finds top-level contrib modules AGAIN » /core/phpunit.xml.dist only finds top-level contrib modules
Crell’s picture

is that contrib would not be able to depend on Drupal's UnitTestBase, or any of the new PHPUnit-based test classes (Kernel and Browser), unless Drupal was declared as a dev dependency in each and every composer.json.

Hm, good point. Making Drupal itself a dev dependency sounds like a terrible idea. :-) I don't know if it's at all feasible to make a drupal-testing package that is just our testing extensions. Honestly that would be a good thing for us to do anyway, just for general code cleanliness, but I don't know if that's possible during RC, or what else it would break.

dawehner’s picture

@Mixologic Did you considered open up an upstream issue for that?

Mixologic’s picture

@dawehner I had thoughts about it, and that would be ideal. --process-isolation should still produce results per test.

Mile23’s picture

I still need to run PHPUnit tests on submodules on the testbot. This is for Examples, specifically phpunit_example.

The testbot uses run-tests.sh, which defers to phpunit.xml.dist for configuration of PHPUnit test suites.

Thus I need to change phpunit.xml.dist if I want the testbot to run tests which teach people how to write tests.

I think it's fair to say that if you want submodule PHPUnit tests, you have to place the module in the top-level modules/ directory.

We limit submodule tests in sites/ based on specific needs having to do with not searching through sites/default/files and so forth, since that would be a security issue.

A wide search through modules/ does not run into this problem.

If a module in modules/ has its own vendor/ directory, then it is Doing Things Wrong. Its composer-based dependencies should live in the root vendor/ directory, having been managed by a composer-based build process.

Also, we have added a composer-based build step to core which removes test directories from vendor/, as a security and space-saving effort.

With all that in mind, here's a patch.

It widens the net on the search through the modules/ directory, and documents why. It also documents that we exclude submodules under sites/ on purpose.

Mile23’s picture

Derp. Editor was set to convert to tabs.

Also: phpunit.xml.dist still excludes core/vendor, which is no longer present. Fixed here to exclude vendor/.

And why not just exclude modules/*/vendor? Done here.

jhedstrom’s picture

+++ b/core/phpunit.xml.dist
@@ -15,11 +15,17 @@
+      <exclude>../modules/*/vendor</exclude>

Wild card exclusion didn't used to be supported in PHPUnit--does this work now?

Mile23’s picture

Well, check right above the patched part... We're including ../sites/*/modules/*/tests/src/Unit.

I just verified this by adding a module under modules/ and putting a failing test in its vendor/ directory. Altering the phpunit.xml.dist file to remove the exclusion, it finds the test. Adding it back, it doesn't.

Mixologic’s picture

The testbots/drupalci do not use phpunit.xml.dist when trying to determine which tests to run. It uses run-tests.sh --directory, which recurses from the root level of the module looking for php files inside of /tests/ and /Tests/ directories.

See: https://www.drupal.org/commitlog/commit/2/a42acf99652f5c82bf937e067acf73...

Mile23’s picture

Thanks, @mixologic.

OK, so that leaves us with the testbot finding files that I can't run locally, leading to a false positive locally.

From the issue for the patch above (#2551981: Add --directory option to run-tests.sh test discovery) @jthorson says:

However, instead of building this scanning capability into the new test runner, the maintainers assert that test discovery should be the responsibility of the test framework itself, not the individual runner ... i.e. the scanning for files should be the responsibility of run-tests.sh, not PIFR or DrupalCI.

Absolutely it should be the responsibility of the test framework, not the runner. But it shouldn't be up to run-tests.sh to ignore PHPUnit configuration, for all the reasons that have led to so many objections in this issue.

So, just to tally, we have:

1) SimpleTest in the UI which *does* discover and run the submodule PHPUnit tests, according to.... policy?

2) run-tests.sh which uses phpunit.xml.dist for test configuration such as strict rules (I think) but which scans for tests everywhere, depending on calling mode.

Which leaves us with:

3) PHPUnit at the command line, which honors whitelist/blacklist from phpunit.xml.dist, meaning it *can't* run tests that the others *can*, for... reasons.

Sadly, the only useful information you'll get about PHPUnit tests comes from running phpunit at the console, which means you're SOL if you want to find out why a test failed. Here's what run-tests.sh tells you when you pass --verbose:

Detailed test results
---------------------


---- Drupal\Tests\phpunit_example\Unit\AddClassTest ----


Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Fail      Other      AddClassTest.php    55 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A
    
Pass      Other      AddClassTest.php     0 Drupal\Tests\phpunit_example\Unit\A

$

So. because all of this is true, someone should RTBC my patch so I can get to work on stuff, please, because apparently the testbot doesn't care.

jhedstrom’s picture

Issue tags: +rc target triage

Confirmed that with the patch in #45 I cannot reproduce the fatal error from #2182231: Testing scans for tests in contrib vendor directory. So sometime in the past year, PHPUnit started supporting wildcard exclusions.

This is tagged as needs tests, but aside from actually adding some placeholder test modules to the /modules directory, I'm not sure what we can do here.

jhedstrom’s picture

Does the patch need updating to include Kernel and Functional tests in addition to Unit?

Mile23’s picture

This is tagged as needs tests, but aside from actually adding some placeholder test modules to the /modules directory, I'm not sure what we can do here.

We can read in phpunit.xml.dist and then parse it and then compare that to what it should be.

Which is stupid.

And seems like a waste of time because none of the testing systems are paying attention to the whitelist or blacklist, so it's a useless file read that no policy cares about.

Which is why I did this: #2607866: [meta] Drupal CI and Testing Config Is Messed Up

neclimdul’s picture

Status: Needs review » Needs work

This has problems when using base classes or traits in the contrib module but otherwise seems to work. I'm guessing we have to tweak something in core/tests/bootstrap.inc as well.

Note: This issue will also cause problems for people using drush make or just drush to install modules as it installs modules in modules/contrib and modules/custom causing there to be no top level modules at all.

Mile23’s picture

Rerunning test in #45.

Mile23’s picture

Status: Needs work » Needs review

Autoloading traits or base classes is definitely the responsibility of bootstrap.php.

phpunit.xml.dist should only care about which tests get run.

Setting back to needs review because I'm not sure what else has to be done. We should file a follow-up if core/tests/bootstrap.php isn't finding some classes or traits.

jhedstrom’s picture

Still wondering about my question in #51 regarding the other tests (kernel + browser) now specified in phpunit.xml.

dawehner’s picture

@jhedstrom Under the assumption that doing this patch is right, we should adapt it for kernel and browser tests, true.

Mile23’s picture

No assumption about it: It's the right thing to do. :-)

This patch changes the kernel and functional test suites to match the unit one.

jhedstrom’s picture

+++ b/core/phpunit.xml.dist
@@ -15,18 +15,26 @@
+      <!-- Exclude vendor directories which may be present in modules/. -->
+      <exclude>../modules/*/vendor</exclude>

Does this need to be repeated for browser and kernel tests?

jhedstrom’s picture

Xano’s picture

Why are we still doing this after the drawbacks have been mentioned in #12 already? When I want to test core, I do not want contrib to be tested as well. It's also a bad idea to assume core has any knowledge of how to run contrib tests. We're needlessly painting ourselves in a corner here.

dawehner’s picture

I have to agree with xano, phpunit.xml files should test stuff of a package, not an entire checkout.

Mile23’s picture

Here's @Xano #12:

I've been shipping phpunit.xml.dist with all my Drupal 8 contrib modules. What's against doing that for all of contrib? Doing so means we won't enforce any directory structure limitations on contrib, which is what any solution to this issue would have to do.

That is the problem this issue solves.

You can easily use the Drupal-installed PHPUnit to go to an arbitrary directory depth within modules/.

Within sites/, a number of issues arise (listed above) for arbitrary depth, so we disallow that. Not a perfect solution, but then what is?

When I want to test core, I do not want contrib to be tested as well. It's also a bad idea to assume core has any knowledge of how to run contrib tests. We're needlessly painting ourselves in a corner here.

Core makes all kinds of assumptions about how to test contrib. In fact, if you look at https://www.drupal.org/node/2499239#comment-10520662 you'll see that the config we're arguing over right now is completely irrelevant to the testbot, or to run-tests.sh.

You are the first to say: I don't want to run contrib during core dev, to which I say: Yay! Someone else uses phpunit instead of run-tests.sh! But also: The behavior you desire (don't run contrib when I'm working on core) is accidental; phpunit.xml.dist makes an effort to run contrib tests, but fails for arbitrary directory depth.

So shall we make a separate test suite for contrib? That would be in contradiction to the needs of others above who want to organize tests by type, not by domain.

I want: The same version of phpunit that core has (so running from DRUPAL_ROOT) , and to be able to run tests filtered by group or testsuite. That way I have the best chance of mimicking the testbot without actually using it.

Xano’s picture

Core makes all kinds of assumptions about how to test contrib.

Making assumptions is what got us in trouble in the first place.

The behavior you desire (don't run contrib when I'm working on core) is accidental; phpunit.xml.dist makes an effort to run contrib tests, but fails for arbitrary directory depth.

The behavior I desire is that we stop doing things we should not be doing. We develop Drupal core, so we must test Drupal core. Testing contributed modules is not our responsibility here.

If someone wants to test their entire application, let them find their own solution. Or maybe we can provide a solution for that, but let's not force everybody to use that solution, especially when we do that in a way that is different from what the rest of the world does.

TLDR; phpunit.xml.dist is for package-level testing. Let's keep it that way.

jhedstrom’s picture

Testing contributed modules is not our responsibility here.

As mentioned in #37 and #41 we still need a way to allow contrib to readily use core's base test classes (unit, kernel, functional/browser). As it stands now, using core's phpunit.xml allows this (if contrib module tests are discoverable).

neclimdul’s picture

2 quick observations. I've read #12 a couple times and it seems the complaint there is the enforced directory structure. While its a sane structure its not actually enforcing it since this isn't how testbot or anything else work.

TLDR; phpunit.xml.dist is for package-level testing. Let's keep it that way.

But it is decidedly not. That's not the way its written and this issue is about fixing/improving and existing feature not adding a new one. You can run tests and I have run tests on contrib and I've repeatedly watched as the devel tests break during the 8.0.0 development because they weren't up to date.

Xano’s picture

As mentioned in #37 and #41 we still need a way to allow contrib to readily use core's base test classes (unit, kernel, functional/browser). As it stands now, using core's phpunit.xml allows this (if contrib module tests are discoverable).

All that requires, is running PHPUnit with --bootstrap ./core/tests/bootstrap.php.

None of the comments actually address the fact that we shouldn't test contributed modules when we actually want to test Drupal core. Having to figure out how to filter tests actually makes the whole workflow more complicated.

Xano’s picture

TLDR; phpunit.xml.dist is for package-level testing. Let's keep it that way.

But it is decidedly not. That's not the way its written and this issue is about fixing/improving and existing feature not adding a new one. You can run tests and I have run tests on contrib and I've repeatedly watched as the devel tests break during the 8.0.0 development because they weren't up to date.

It is how most of the PHP world uses phpunit.xml.dist. Let's stick to that convention. There's no reason to deviate here. We're trying to get off our island, not dig ourselves in. This also means anyone wanting to test core can just test core.
If we also want to provide application-level testing, we should ship with an additional application-level configuration file for PHPUnit. I'm still not entirely sure what the use cases for that are, but I'm not against including this functionality, as long as it does not interfere with more basic and common functionality.

Mile23’s picture

The point here is that we need to replicate the testbot as much as possible while using phpunit at the command line, which for me means using the executable in vendor/, and a Drupal-supplied config file.

So if you want to avoid core when running contrib and vice-versa, then as @neclimdul points out, that's a feature request. We could maybe do it with a contrib.phpunit.xml file, though that didn't get any traction when I mentioned it above in #38.

Xano’s picture

The point here is that we need to replicate the testbot as much as possible while using phpunit at the command line, which for me means using the executable in vendor/, and a Drupal-supplied config file.

What exactly do we need to replicate, and why?

So if you want to avoid core when running contrib and vice-versa, then as @neclimdul points out, that's a feature request

It's not a feature. It's fixing buggy behavior by removing wrong assumptions and replacing them with dedicated, targeted solutions.

Let's put it this way: if someone creates a suite of modules that has no modules subdirectory, top-level directories per module, then this patch just breaks because it makes the wrong assumptions.

Mile23’s picture

What exactly do we need to replicate, and why?

I want to run unit tests with PHPUnit. I want to generate coverage reports with PHPUnit. I want to do code quality stuff with PHPUnit. I want to automate this process. I don't want a lot of special cases to deal with during contrib development. I want my tests to be run on the same PHPUnit executable that they'll run with on the testbot. I want the same strictness level as the testbot.

Also, currently, #2578721: [examples] Drupal Examples for Developers is somewhat blocked by having only this hobbled PHPUnit config file to work with.

Let's put it this way: if someone creates a suite of modules that has no modules subdirectory, top-level directories per module, then this patch just breaks because it makes the wrong assumptions.

Right, so if you run the test using the patched config and expect it to work within a module directory instead of DRUPAL_ROOT, then yes, you have problems. But it's the same problem as if you expected that without the patch.

So, as you say, you'd add --bootstrap 'path/to/boostrap.php' to get autoloading, and maybe add your own phpunit.xml within the module directory. Which is what you're already doing.

Not sure how that's a problem, because it's what you seem to want.

This patch fixes the behavior of finding all the submodules in DRUPAL_ROOT/modules/, so if you decide to change your ways, and run phpunit from DRUPAL_ROOT, put your suite of modules in DRUPAL_ROOT/modules, and use --group to filter for your submodule, it will actually work, instead of not finding your tests.

Crell’s picture

Sounds like we need at least semi-formal user stories here, frankly, because we're all talking about different things. At which point this issue should get retitled/resummarized, since it becomes about more than the stated bug.

Mixologic’s picture

Sounds like we need at least semi-formal user stories here

+1 to that.

I want my tests to be run on the same PHPUnit executable that they'll run with on the testbot

The best way to achieve this is to get the testbots to execute unit tests separately from simpletests, so that the testbots run the PHPUnit executable.

That is currently blocked on the fact that the PHPUnit executable is an inadequate tool from a CI perspective. It needs to be able to gracefully handle segfaults when running in isolated process mode. Graceful handling of segfaults means that if a test aborts, we get a structured response that tells us which test failed. If we can fix that upstream, we can get the testbot to use phpunit the same way as people using it on the commandline use it.

dawehner’s picture

That is currently blocked on the fact that the PHPUnit executable is an inadequate tool from a CI perspective. It needs to be able to gracefully handle segfaults when running in isolated process mode.

Did you created an issue on phpunit itself about that?

Mixologic’s picture

I have not. run-tests.sh already does what we need from a testbot perspective, and I do not have any spare cycles to try and improve phpunit upstream.

Mile23’s picture

Issue tags: -rc target triage

#2638290: Create stream_wrapper_example can't happen because of this issue.

PHPUnit can't find either of the two tests that example defines.

The testbot finds both tests.

run-tests.sh can only find the Kernel test when I give it the group name. But when I tell it the class name of the Unit test, it discovers the test and runs it.

Drupal 8 UI only finds the one test and fails it, even though it's passing.

There is no way I can feel alright about these tests under this circumstance.

See: #2607866: [meta] Drupal CI and Testing Config Is Messed Up

Mile23’s picture

Issue summary: View changes
FileSize
2.84 KB
1.16 KB

Modified the issue summary because any other problems seem to have been solved.

There is no down-side.

Patch in #58, updated to reflect #59, which also fixes existing problems with those test suites. (Excluding ./vendor instead of ../vendor)

Once again: The *ONLY* change this makes is to allow the PHPUnit command line tool to find unit tests in sub-modules under DRUPAL_ROOT/modules.

It continues to be strict about submodule test discovery under sites/, which is a security concern (because we'd be scanning user-supplied content for tests).

The reason we'd be doing this is to allow anyone to type $ ./vendor/bin/phpunit --group modulename and test their module without setting up a whole CI process and one-off test runner, though those would of course still be an option.

Work continues on improving PHPUnit-based test results in the UI and run-tests.sh to provide even *more* useful options: #2427191: Test-runner ignores @group annotations for PHPUnit-based tests in modules. #2608532: Simpletest UI munges PHPUnit-based test output #2607866: [meta] Drupal CI and Testing Config Is Messed Up

wiifm’s picture

After battling with Drupal 8 and PHPunit, and quite frankly extremely misleading documentation for several hours, I stumble across http://drupal.stackexchange.com/questions/162719/phpunit-with-custom-con... which in turns leads here.

In my particular scenario, I had my custom modules under a directory /modules/custom/my_module.

Before patch (stock Drupal 8.0.4) - my PHPunit tests are not recognised, I nearly go crazy.

After patch #77 - my PHPunit tests are magically found, I rejoice.

This is a huge +1 from me for RTBC, this will improve the DX a lot, and reduce worldwide developer frustration measurably.

Xano’s picture

alexpott’s picture

Also over in #2329453: Ignore front end vendor folders to improve directory search performance we've been discussing optimisations to directory search when module have javascript assets which can lead to thousands of files in a module. With the fix here we're going to be scanning them in order to test.

neclimdul’s picture

I'm not sure I understand the connection to the npm/bower issue.

alexpott’s picture

@neclimdul phpunit is going to have to scan all of those directories if we make this change.

Mile23’s picture

It already scans those directories for top-level modules.

That's one reason why #2301873-8: Use ExtensionDiscovery to register test namespaces for running PHPUnit might be a good idea.

bojanz’s picture

I agree with Xano, I have no expectation for Core's phpunit.xml to discover contrib tests.
All my scripts point phpunit to the exact contrib folder (such as modules/commerce), which in turn discovers any submodule unit tests.

That said, #77 fixes the existing behavior under the existing assumptions, so I'm fine with that getting in while we converge on the better idea.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

It already scans those directories for top-level modules.

No, HEAD does not scan outside of a module's tests directory.

The currently implementation also completely breaks the testsuite implementation meaning that phpunit --testsuite Functional will run contrib module's Unit, Kernel and FunctionaJavascript too...

alexpott’s picture

+++ b/core/phpunit.xml.dist
@@ -20,33 +20,47 @@
-      <exclude>./vendor</exclude>
+      <exclude>../vendor</exclude>
...
-      <exclude>./vendor</exclude>
+      <exclude>../vendor</exclude>
...
-      <exclude>./vendor</exclude>
+      <exclude>../vendor</exclude>

This doesn't need changing it can just be removed - it'll never be found anyway.

Mile23’s picture

The currently implementation also completely breaks the testsuite implementation meaning that phpunit --testsuite Functional will run contrib module's Unit, Kernel and FunctionaJavascript too...

HEAD does this:

$ ./vendor/bin/phpunit -c core/ --testsuite Functional
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.



Time: 485 ms, Memory: 2.50Mb

No tests executed!

...note lower-case f... :-)

$ ./vendor/bin/phpunit -c core/ --testsuite functional
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.

EEEE

Time: 1.96 seconds, Memory: 3.00Mb

There were 4 errors:

We also still see this:

$ drush dl examples
Project examples (8.x-1.x-dev) downloaded to                         [success]
/Users/paulmitchum/projects/drupal8//modules/examples.

$ ./vendor/bin/phpunit -c core --group phpunit_example
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.



Time: 13.6 seconds, Memory: 217.75Mb

No tests executed!
alexpott’s picture

@Mile23 sorry I messed up the casing of my command but that was hardly the point. The point it that with this change trying to limit by test suite won't work for contrib modules.

Mile23’s picture

Assigned: Unassigned » Mile23

Oh, I thought you were referring to HEAD not working, because you said 'current implementation.'

Gottit.

PHPUnit here just grabs all the test files it can find based on the testsuite's directory and exclude rules. We ignore those rules in TestDiscovery which Simpletest and UI use, which means it's easy for them to drift apart.

bootstrap.php sets up the autoloader to be able to find tests in contrib mapping like this: $namespaces['Drupal\\Tests\\' . $extension . '\\'][] = $dir . '/tests/src'; This is again different from TestDiscovery which (currently) silos different test types by namespace, for a different behavior in Simpletest and UI.

TestDiscovery finds tests in project subdirectories, whereas phpunit.xml.dist does not.

This leads us back to #2605664-123: [Needs change record updates] Align TestDiscovery and bootstrap.php's non-permissive loading of PSR-4 namespaces for traits where @alexpott lays out principles for design in Drupal tests. The basic desire is to have all the different test tools run the same tests under similar circumstances. So running phpunit --group modulename should run the same tests as run-tests.sh --module modulename and run-tests.sh modulename.

So: Is TestDiscovery right, or is bootstrap.php + phpunit.xml.dist?

I'd say TestDiscovery is correct, and we should find tests in arbitrary modules because that's how run-tests.sh works, and that's our main testing standard for Drupal 8.

So to do that in phpunit.xml.dist, we have to re-write the config to search in arbitrary directories under module/ but not sites/ for security reasons.

This brings the phpunit tool config into harmony with TestDiscovery, but creates the problem from #86: When the user specifies a testsuite, we lack a way to express which types of test to run at an arbitrary directory depth, so the command will run all the tests.

So now we're outside the realm of test suites we can configure through XML if we want phpunit's --group argument to behave like run-test.sh's --module.

This leaves us with some options:

  • Don't care.
  • Write suite loaders for phpunit.

It turns out we can look to CakePHP for an example of a suite loader for tests with hard dependencies: https://github.com/cakephp/cakephp/blob/master/phpunit.xml.dist The "Database Test Suite" specifies a test suite by PHP file rather than directory, which is some code that runs to determine which tests belong to the suite.

The suite runner is here: https://github.com/cakephp/cakephp/blob/master/tests/TestCase/DatabaseSu... which implements suite(). suite() adds tests to the suite based on whatever logic is desired, and then PHPUnit runs them.

So:

  • Add \Drupal\Core\TestSuite\UnitTestSuite, FunctionalTestSuite, etc. In practical terms, probably a base suite class with subclasses.
  • Configure phpunit.xml.dist to figure out test suites based on those classes.
  • Suite classes should be flexible enough to discover tests in the context of TestDiscovery/run-tests.sh as well so we can make these decisions in a central place. (That's a follow-up.)

Is this some work I should get to? OK, since you said yes.

alexpott’s picture

So myabe we use TestDiscovery in phpunit.xml.dist - that'd actually be super helpful.

Mile23’s picture

Status: Needs review » Needs work

It should go the other way around. We want no dependencies in our suite loaders, because we're using them to test the dependencies we would have.

Also, I hijacked this issue #2301873: Use ExtensionDiscovery to register test namespaces for running PHPUnit which was originally about using TestDiscovery. It's from the days before all the different test suites, however, so it's pretty well stale.

Mile23’s picture

Title: /core/phpunit.xml.dist only finds top-level contrib modules » Use test suite classes to discover different test types under phpunit
Version: 8.1.x-dev » 8.2.x-dev
Assigned: Mile23 » Unassigned
Status: Needs work » Needs review
FileSize
5.28 KB

OK, so here we go.

This is a proof-of-concept which only deals with the unit test suite.

It does use TestDiscovery's static methods, which is adequate for now. The problem is that TestDiscovery is in the Drupal\simpletest namespace, so whether it's autoloadable during test time is up for grabs, given recent conversations. At the moment it works, however. Also those static methods are static for a reason and could easily be refactored to some other location such as these suite loaders.

This patch adds UnitTestSuite, which implements the suite() method, which magically gives it the power to define a test suite for PHPUnit. It performs some file scanning on places in core and contrib known to have unit tests. PHPUnit then takes the classes we give it and does further checking with reflection to make sure everything's cool and to determine @groups and so forth.

UnitTestSuite is outside the realm of autoloadable classes, in keeping with the minimal-autoload footprint approach. It could easily move somewhere more autoloadable, because: When you call UnitTestSuite::suite() you'll be supplied with an iterator of all available unit tests. :-) This has to come in handy for other testing systems.

After the change, --testsuite unit runs the same number of tests as before. And it passes the examples test, namely it finds @group phpunit_example. :-)

Running without any group our testsuite argument results in the same number of tests before and after.

If this doesn't look scary and awful to people I'll optimize a bit and move on to the other suites.

alexpott’s picture

@Mile23++ nice work. This will make everyone's life easier.

Autoloading from \Drupal\simpletest\src is fine - the bit of autoloading I question is - \Drupal\simpletest\tests\src - because if you enable autoloading from there you are changing the run-time behaviour as that is not normally available. The first location can be autoloaded from the moment the module is installed. Also given the relative locations of all the files are known - we can just do an include if we decide to not allow this to be autoloaded at this point.

+++ b/core/tests/TestSuites/UnitTestSuite.php
@@ -0,0 +1,53 @@
+    $root = dirname(dirname(dirname(__DIR__)));

+++ b/core/tests/bootstrap.php
@@ -34,11 +34,13 @@ function drupal_phpunit_find_extension_directories($scan_directory) {
-function drupal_phpunit_contrib_extension_directory_roots() {
-  $root = dirname(dirname(__DIR__));
+function drupal_phpunit_contrib_extension_directory_roots($root) {

It'd be nice to do it first without an API change. We can do the root passing around later. Yes it aids testability - but let's do that in a followup. Because atm its still quite hard-coded.

Mile23’s picture

Title: Use test suite classes to discover different test types under phpunit » Use test suite classes to discover different test types under phpunit, allow contrib harmony with run-tests
FileSize
12.46 KB

The first location can be autoloaded from the moment the module is installed.

The module is never installed in this scenario, except when running a test that installs it. But bootstrap.php loads the non-test namespace for all extensions, so we're good I guess. If we decide to change this policy, we can move/refactor TestDiscovery.

Round 2.

No longer injecting $root into the bootstrap functions. I'd argue that there's no way functions in bootstrap could be considered API, but yah.

Each of the suite classes is very similar. One might say they look copy-pasted. :-) However each discovery method is just different enough that it would be more complex to normalize them into a single function with a calling signature, etc. Also, needs may change later on and it will be easier to know what to change this way. So some naive code there is probably best.

Each suite class has a protected helper method like getUnitTests(). This was to allow for testing, but since we're not injecting $root, that will come later.

Mile23’s picture

FileSize
13.19 KB

Forgot an interdiff...

Also just to mention that the thought process here is in #90. #2499239-90: Use test suite classes to discover different test types under phpunit, allow contrib harmony with run-tests

neclimdul’s picture

With the exception of the Unit tests, we can actually narrow this down to one method they can share similar to cake. DRY! :)

neclimdul’s picture

I really like this patch BTW, makes maintaining phpunit.xml much easier and provides a really handy tool for site builders building their own test suites.

I don't think it does anything for #2605664: [Needs change record updates] Align TestDiscovery and bootstrap.php's non-permissive loading of PSR-4 namespaces for traits or anything for TestDiscovery at all but that's fine with me. :)

edit: fix issue number

neclimdul’s picture

+++ b/core/modules/views/tests/src/Kernel/ViewsKernelTestBase.php
@@ -11,7 +11,7 @@
-class ViewsKernelTestBase extends KernelTestBase {
+abstract class ViewsKernelTestBase extends KernelTestBase {

+++ b/core/tests/Drupal/Tests/Core/Render/RendererTestBase.php
@@ -22,7 +22,7 @@
-class RendererTestBase extends UnitTestCase {
+abstract class RendererTestBase extends UnitTestCase {

For anyone coming along and wondering what these have to do with anything like I did, phpunit skipped adding tests that didn't have any test methods at all but the way we're explicitly adding them these get run. This causes new failures in the phpunit runner. That is a distinct change in the discovery ATM.

jhedstrom’s picture

Issue summary: View changes

I manually tested this approach and it finds tests in the root modules directory as expected.

Updating the IS to reflect how to manually test (now that vendor isn't in the core directory).

  1. +++ b/core/phpunit.xml.dist
    @@ -28,48 +28,16 @@
    -      <directory>./tests/Drupal/FunctionalTests</directory>
    -      <directory>./modules/*/tests/src/Functional</directory>
    -      <directory>../modules/*/tests/src/Functional</directory>
    -      <directory>../profiles/*/tests/src/Functional</directory>
    -      <directory>../sites/*/modules/*/tests/src/Functional</directory>
    -      <!-- Exclude Composer's vendor directory so we don't run tests there. -->
    -      <exclude>./vendor</exclude>
    -      <!-- Exclude Drush tests. -->
    -      <exclude>./drush/tests</exclude>
    +      <file>./tests/TestSuites/FunctionalTestSuite.php</file>
    

    I love this simplification of the xml file!

  2. +++ b/core/tests/Drupal/Tests/Core/Render/RendererTestBase.php
    @@ -22,7 +22,7 @@
    +abstract class RendererTestBase extends UnitTestCase {
    

    I'm guessing these changes to abstract are because the new approach would 'discover' these classes? One question though, why doesn't our old discovery also find them?

jhedstrom’s picture

cross post there, thanks for the clarification on the change to abstract.

Mile23’s picture

Status: Needs review » Needs work

Ossum. :-)

  1. +++ b/core/tests/TestSuites/FunctionalJavascriptTestSuite.php
    @@ -15,47 +12,16 @@ class FunctionalJavascriptTestSuite extends TestSuiteBase {
        *
    -   * @return static
    +   * @return self
        *   The test suite.
    
    +++ b/core/tests/TestSuites/UnitTestSuite.php
    @@ -15,46 +12,16 @@ class UnitTestSuite extends TestSuiteBase {
        *
    -   * @return static
    +   * @return self
        *   The test suite.
    

    It's static. I looked it up: https://www.drupal.org/coding-standards/docs#types :-)

  2. +++ b/core/tests/TestSuites/TestSuiteBase.php
    @@ -12,15 +13,8 @@ class TestSuiteBase extends \PHPUnit_Framework_TestSuite {
        *   Associative array of extension paths, with extension name as keys.
    -   *
    -   * @todo Allow injecting root into
    -   *   drupal_phpunit_contrib_extension_directory_roots() so that we can be sure
    -   *   we're scanning the same directory as the rest of the suite loaders.
        */
       public static function findExtensionDirectories() {
    

    This wouldn't be a @todo except @alexpott said to do it in a follow-up. Can we leave the @todo?

  3. +++ b/core/tests/TestSuites/TestSuiteBase.php
    @@ -29,4 +23,35 @@ public static function findExtensionDirectories() {
    +  protected function addSuiteNamespace($root, $suite_namespace) {
    

    This isn't adding a namespace. It's adding files to the test suite.

jhedstrom’s picture

Can we leave the @todo?

I think new @todos are only allowed if they link to an issue.

neclimdul’s picture

Hah, I didn't mean to take over the patch, just was breaking it down the patch to understand it and tossed the changes back up. :)

1) That is ridiculous and in direct contradiction to PSR-5's types. Maybe its just vague... WTF? static and self have 2 distinct meanings so that seriously needs to be fixed. ATM it doesn't matter so rather then fight it just have the return type not match the documented type I changed the instantiation too... seriously WTF though?
2) Sure, I missed Alex's comment. Like jhedstrom said it should get added back with the link, feel free to do so.
3) err... yeah I think I just grabbed cake's method to start with and replaced some words and never read it. That's quite odd for sure.

dawehner’s picture

I really like the elegance of that solution ...

+++ b/core/tests/TestSuites/FunctionalJavascriptTestSuite.php
@@ -0,0 +1,27 @@
+/**
+ * Discovers tests for the unit test suite.
+ */
+class FunctionalJavascriptTestSuite extends TestSuiteBase {

+++ b/core/tests/TestSuites/FunctionalTestSuite.php
@@ -0,0 +1,27 @@
+/**
+ * Discovers tests for the unit test suite.
+ */
+class FunctionalTestSuite extends TestSuiteBase {
...
+   * Factory method which loads up a suite with all unit tests.

+++ b/core/tests/TestSuites/KernelTestSuite.php
@@ -0,0 +1,27 @@
+   * Factory method which loads up a suite with all unit tests.

Nitpick: These comments are wrong.

Random fact: 4 months ago we had 14k tests, now we have 18k tests? This is a bit odd

Mile23’s picture

Status: Needs work » Needs review

Setting NR to run testbot.

Mile23’s picture

Addressing #105 and a few others I found.

Follow-ups:

  • Inject root into the drupal_phpunit_*() functions.
  • Write tests.
Mile23’s picture

Issue summary: View changes

Updating issue summary.

Mixologic’s picture

Random fact: 4 months ago we had 14k tests, now we have 18k tests? This is a bit odd

Thats due to http://cgit.drupalcode.org/drupal/tree/core/tests/Drupal/Tests/Component...

For some strange reason the test author imagined that we really needed to test every single one of these possibilities. Fortunately they're super fast and didnt increase testing load too much, but yeah..

Mile23’s picture

Version: 8.2.x-dev » 8.1.x-dev

Since this is a bug report and it only improves tests I'm setting to 8.1.x. The patch applies to either branch. Re-running the test on #107.

Mile23’s picture

Patch still applies to 8.1.x.

As per #94 the patch in #107 still doesn't inject root into the bootstrap functions, which precludes testing and is kind of iffy.

alexpott’s picture

precludes testing and is kind of iffy.

How about adding a test which adds a class which extends TestSuiteBase and allows you to specific the root to test? That way we get the important test coverage of the logic in TestSuiteBase:: addTestsBySuiteNamespace().

alexpott’s picture

Also rather than breaking BC we could just make $root optional - so continue with the current behaviour if it's not supply but also make testing easy.

Mile23’s picture

Turns out it doesn't matter anyway for testing, because drupal_phpunit_find_extension_directories() uses SplFileInfo->getRealPath(), which can't deal with stream-based filesystems. Thus we can't test using mocked file systems.

So we can have the optional $root parameter to drupal_phpunit_contrib_extension_directory_roots(), which won't change the API (if you say so...) but which guarantees that we're scanning the same directory as the test suite class. This makes me happy. :-)

We have a test of the special case for core Unit tests, which is most relevant.

Room for improvement will be a test for namespace/path manipulation for extensions (minus the discovery part, for reasons given above). Which I'll do when I get to TCDrupal which I'm an hour late for right now because I was doing this instead. :-)

alexpott’s picture

Like the test! You're right it is nice to have one :)

which won't change the API (if you say so...)

Well it is a permitted API change because we have a BC layer.

FILE: .../drupal/core/tests/Drupal/Tests/TestSuites/TestSuiteBaseTest.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 5 | WARNING | [x] Unused use statement
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Could be fixed on commit.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I really think this is simply beautiful! You know these moments when you nearly wanna cry for happiness? Its not exactly that, but its getter there.

I wasn't sure, but this test change seems really like a detail which is personal preference, so RTBC.

+++ b/core/tests/Drupal/Tests/TestSuites/TestSuiteBaseTest.php
@@ -0,0 +1,135 @@
+
+    // Access addTestsBySuiteNamespace().
+    $ref_add_tests = new \ReflectionMethod($stub, 'addTestsBySuiteNamespace');
+    $ref_add_tests->setAccessible(TRUE);
+
+    // Invoke addTestsBySuiteNamespace().
+    $ref_add_tests->invokeArgs($stub, [vfsStream::url('root'), $suite_namespace]);
...
+class StubTestSuiteBase extends TestSuiteBase {
+

This is a bit confusing. We have a stub class but then still call out to protected methods via invokeArgs. What about improving the readability by making these methods public just for the test.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

@dawehner I think I prefer your way too but the benefit of this far outweighs personal preferences.

Committed 492728a and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed 765aa9e on 8.2.x
    Issue #2499239 by Mile23, neclimdul, alexpott, jhedstrom, Mixologic,...

  • alexpott committed 492728a on 8.1.x
    Issue #2499239 by Mile23, neclimdul, alexpott, jhedstrom, Mixologic,...
Mile23’s picture

Status: Fixed » Needs review
FileSize
3.56 KB

Hehe....

I was still working. :-)

Rebased with the commit, patch adds tests.

  • alexpott committed 765aa9e on 8.3.x
    Issue #2499239 by Mile23, neclimdul, alexpott, jhedstrom, Mixologic,...

  • alexpott committed 765aa9e on 8.3.x
    Issue #2499239 by Mile23, neclimdul, alexpott, jhedstrom, Mixologic,...
Mile23’s picture

Status: Needs review » Fixed
Related issues: +#2785025: Expand testing for TestSuiteBaseTest

Status: Fixed » Closed (fixed)

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

klausi’s picture

Created #2794715: TestSuiteBaseTest cannot be executed as standalone test as a follow-up.

Unfortunately these TestSuite classes are not suitable to run with Paratest, so I created a script to generate a phpunit.xml file for parallel testing on Travis CI: https://github.com/klausi/drupal/blob/8.3.x-phpunit-travis/travis/testru...

Mile23’s picture

This issue now has a change record, which it should have had all along: https://www.drupal.org/node/2799437

Mile23’s picture

Finally getting back to the caveat about TestDiscovery being in \Drupal\simpletest from #93: #2863055: Move TestDiscovery out of simpletest module, minimize dependencies