Use test suite classes to discover different test types under phpunit, allow contrib harmony with run-tests

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

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.

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.

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.)

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

That is scary.

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 Ehem?

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.

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.

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-...

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.

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.

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"?

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?

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.

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?

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.

#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.

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.

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)

#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.

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?

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.

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.

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.

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.

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.

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.

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.

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

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

+++ 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?

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...

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.

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

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.

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

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

+++ 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?

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.

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

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.

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).

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.

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.

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.

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.

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.

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.

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?

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.

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.

#2677122: Allow ./core/phpunit.xmldist to run package tests only addresses the concerns raised in #12.

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.

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

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

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.

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...

+++ 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 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.

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

@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.

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

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

+++ 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.

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?

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

Can we leave the @todo?

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

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.

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

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..

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().

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.

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.

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.

@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!

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...