Move TestSuite/* into a dedicated namespace

Marking this as a duplicate of #2794715: TestSuiteBaseTest cannot be executed as standalone test since it's two errors requiring the same solution. If we decide something else there, we can re-open here.

In #2794715-10: TestSuiteBaseTest cannot be executed as standalone test it was decided that the solution to this issue would be too disruptive for 8.2.x, so if we're going to move files around in namespaces we should do that for 8.3.x.

Here's the patch from #5 in that issue. It will likely need to be re-rolled after that issue is committed.

Thanks @Mile23; I agree with splitting the changes this way. I think a change record for this change would probably be good?

OK so this patch moves the test suite classes to \Drupal\TestSuites\* and has the test in \Drupal\Tests\TestSuites\TestSuiteBaseTest.

It changes the test so that it doesn't need require_once as per #2794715: TestSuiteBaseTest cannot be executed as standalone test

We can still run TestSuiteBaseTest as a single test:

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

..

Time: 20.2 seconds, Memory: 86.00Mb

OK (2 tests, 4 assertions)

Change record coming up. We should have made a change record for #2499239: Use test suite classes to discover different test types under phpunit, allow contrib harmony with run-tests so I'll include that.

Someone asked for a change record? https://www.drupal.org/node/2799437

Failed test is FeedAdminDisplayTest, which I'm willing to bet is unrelated. Resetting to RTBC and restarting testbot.

+++ b/core/tests/Drupal/Tests/TestSuites/TestSuiteBaseTest.php
@@ -2,14 +2,11 @@
-// The test suite class is not part of the autoloader, we need to include it
-// manually.
-require_once __DIR__ . '/../../../TestSuites/TestSuiteBase.php';

+++ b/core/tests/bootstrap.php
@@ -117,6 +117,7 @@ function drupal_phpunit_populate_class_loader() {
+  $loader->add('Drupal\\TestSuites', __DIR__);

We need to be changing the autoloader less under testing not more. The test suite classes have no need to be autoloaded.

Agreed, can we just move the test suite classes to core/tests/Drupal/Tests?

I know the test suites are not unit tests, but that way we avoid modifying the auto loader yet again and can still remove the require_once statements.

can we just move the test suite classes to core/tests/Drupal/Tests

Not sure why that's needed. @alexpott is talking about not allowing autoloading for the test suites *at all.* It's a good ideal.

+++ b/core/tests/bootstrap.php
@@ -117,6 +117,7 @@ function drupal_phpunit_populate_class_loader() {
   $loader->add('Drupal\\KernelTests', __DIR__);
   $loader->add('Drupal\\FunctionalTests', __DIR__);
   $loader->add('Drupal\\FunctionalJavascriptTests', __DIR__);
+  $loader->add('Drupal\\TestSuites', __DIR__);

so this line should not be added?

I find this require_once hackery in all the classes annoying. Any Drupal contrib module that wants to use TestSuiteBase has to do some require_once weirdness yet again :(

TestSuiteBase is not an extension point. How exactly is a contrib module going to use TestSuiteBase?

@klausi the line should not be added because changing the autoloader because of the test system is wrong. The autoloader should match the environment without the test system in play.

Ewps... Missed changing that one line in bootstrap for the last patch.

As for contrib using the suite classes.. I'm not sure what the use-case is. I'd really rather see more of core use these, though, for uses like the UI runner. If that need arises, we can maybe just change it back.

The autoloader should match the environment without the test system in play.

If that's the case, then there's a logical follow-up to modify bootstrap.php to not change the autoloader. But, you know... good luck. :-) We have to discover extension tests in arbitrary locations.

Restarting test after CI error.

Patch no longer applies. It looks very good though and has been rtbc 3 times already.

Reroll.

Also, the require_once would mean that the class was hanging around for all subsequent tests, so I refactored TestSuiteBaseTest a little to use a mock instead of a subclass, and added @runTestsInSeparateProcesses.

OK, so if we isolate the test system from the system under test, then the test listener can't find TestSuiteBase for @coversDefaultClass, as the fail in #26 shows:

Drupal\Tests\Core\Test\TestSuiteBaseTest::testAddTestsBySuiteNamespaceCore with data set "unit-tests" (array(array(array(), array(), array(array(array('<?php'), array('<?php', array('<?php', array('<?php'))))))), 'Unit', array('vfs://root/core/tests/Drupal/...st.php'))
@coversDefaultClass does not exist '\Drupal\TestSuites\TestSuiteBase': Drupal\Tests\Core\Test\TestSuiteBaseTest::testAddTestsBySuiteNamespaceCore with data set "unit-tests"

This tests passes locally for me because I use --testsuite unit to run it, which loads TestSuiteBase in order to do discovery.

Keeping the classloader isolation seems to be an important restriction from #16 and #17, so we'll remove @coversDefaultClass/@covers and turn them into documentation.

#28 and the reasons for removing @covers sounds like a super solid idea.

+++ b/core/phpunit.xml.dist
@@ -37,16 +37,16 @@
-      <file>./tests/TestSuites/UnitTestSuite.php</file>
+      <file>./tests/Drupal/TestSuites/UnitTestSuite.php</file>

Won't these changes break everyone's customised phpunit.xml files? I think we shouldn't make this change in this way.

Agree with @alexpott this will break local uniting testing.

Test suites were dropped from PHPUnit 10. We deprecated them during the Drupal 10 cycle and removed from Drupal 11, CR: https://www.drupal.org/node/3405829. TestSuiteBaseTest still exists, but no longer has the include statement because the target class was deleted. Closing as outdated.