RegistryTest, RegistryLegacyTest both define the same class, use mock instead

Since the issue of PHPUnit CLI testing breaking is recurring quite frequently, I thought it would be good to add a test to prevent regressions. The problem is that DrupalCI, through run-tests.sh, runs tests one at a time indicating a test file at each run. That prevents PHPUnit's own test discovery mechanism to execute. However, that is exactly the mechanism that runs at the time of testing through CLI (which we are recommending more and more over run-tests.sh, btw), and that mechanisms autoload classes across different files - and that's when collisions happen.

The test here is inspired by BuildTestBase.

#6:

I'd suggest that we can do the test discovery part without the test running part by calling phpunit -c core --list-tests

Done that.

it could be a build test

I tried that but failed to make it work... anyone feel free to take it on if you deem it relevant.

Sorry.

+++ b/core/tests/Drupal/Tests/Core/Test/PhpUnitCliTest.php
@@ -0,0 +1,29 @@
+/**
+ * @group Test
+ */
+class PhpUnitCliTest extends UnitTestCase {
+
+  /**
+   * Tests that running tests via the PHPUnit CLI works properly.
+   */
+  public function testPhpUnitCliRun() {
+    $process = new Process('vendor/bin/phpunit --configuration core --verbose --list-tests');
+    $process->setWorkingDirectory($this->root)
+      ->setTimeout(300)
+      ->setIdleTimeout(300);
+    $process->run();
+    $this->assertEquals(0, $process->getExitCode(),
+      'COMMAND: ' . $process->getCommandLine() . "\n" .
+      'OUTPUT: ' . $process->getOutput() . "\n" .
+      'ERROR: ' . $process->getErrorOutput() . "\n"
+    );
+  }
+
+}

I don't think this is the right approach. Running all our unit tests again to prove that they are all runnable as one feels way off. Either we do that or we do tun-tests.sh but doing both feels very Drupal.

Also why is this a critical task. If running a single PHPUnit test from the command line was broken I'd agree but it is not. This are only broken when you run core/tests/Drupal/Tests/Core/Theme/RegistryLegacyTest.php and core/tests/Drupal/Tests/Core/Theme/RegistryTest.php in the same PHPUnit run.

#12 with --list-tests we just discover the tests, without running them. Test discovery is enough to autoload all test classes by PHPUnit CLI and show the problem.

why is this a critical task

right - major given Cause test failures in environments not supported by the automated testing platform. in Priority levels of issues.

Anyway, this comes from #3109480: Properly deprecate theme functions for Drupal 10 which is a committed critical, half way being backported to 8.x.

+++ b/core/tests/Drupal/Tests/Core/Test/PhpUnitCliTest.php
@@ -0,0 +1,29 @@
+
+  /**
+   * Tests that running tests via the PHPUnit CLI works properly.
+   */
+  public function testPhpUnitCliRun() {
+    $process = new Process('vendor/bin/phpunit --configuration core --verbose --list-tests');
+    $process->setWorkingDirectory($this->root)
+      ->setTimeout(300)
+      ->setIdleTimeout(300);
+    $process->run();
+    $this->assertEquals(0, $process->getExitCode(),
+      'COMMAND: ' . $process->getCommandLine() . "\n" .
+      'OUTPUT: ' . $process->getOutput() . "\n" .
+      'ERROR: ' . $process->getErrorOutput() . "\n"
+    );
+  }
+

I don't think this is helpful as a core test, it is extremely meta. If we really want to add this as a linting thing, we could add it to DrupalCI itself?

+++ b/core/tests/Drupal/Tests/Core/Test/PhpUnitCliTest.php
@@ -0,0 +1,35 @@
+class PhpUnitCliTest extends UnitTestCase {

You're my hero...Not the prettiest or most complete test but that covers a solid basic lint that will be very helpful and a complete test would be using the runner so yeah... :)

@catch I'd love for DrupalCI to do phpunit testing but after like 5 years of my CI being randomly broken and pushing for using the phpunit runner in testing discussions with no movement.. for things like this I'd love to have at least this basic test. The right fix is such a big lift and this is small and should save a lot of headaches from a class of bugs.

This fixes things so I can run tests and coverage for core issues again, avoids regressions, and passes so RTBC imho.

+++ b/core/tests/Drupal/Tests/Core/Theme/RegistryLegacyTest.php
@@ -152,29 +152,18 @@ public function testGetLegacyThemeFunctionRegistryForModule() {
-  return RegistryLegacyTest::$functions ?: \get_defined_functions();

As we are removing this I think we can get rid of all references to $functions in the RegistryLegacyTest class as well.

+1 to adding PhpUnitCliTest to avoid this sort of copy paste error in the future, this was my fault originally so apologies for that :)

I'm not a huge fan of the test. Because I fear that it'll add noise to the test runs doing double fails. But as it is only listing tests I guess it is better than nothing. I dunno I guess time will tell.

Committed and pushed 0a65cd6076 to 9.0.x and ad2edda33d to 8.9.x. Thanks!

I cleaned up $functions in the legacy test as this change removes its purpose.

diff --git a/core/tests/Drupal/Tests/Core/Theme/RegistryLegacyTest.php b/core/tests/Drupal/Tests/Core/Theme/RegistryLegacyTest.php
index e8a261b4a9..6e00b48440 100644
--- a/core/tests/Drupal/Tests/Core/Theme/RegistryLegacyTest.php
+++ b/core/tests/Drupal/Tests/Core/Theme/RegistryLegacyTest.php
@@ -69,13 +69,6 @@ class RegistryLegacyTest extends UnitTestCase {
    */
   protected $themeManager;
 
-  /**
-   * The list of functions that get_defined_functions() should provide.
-   *
-   * @var array
-   */
-  public static $functions = [];
-
   /**
    * {@inheritdoc}
    */
@@ -92,14 +85,6 @@ protected function setUp() {
     $this->setupTheme();
   }
 
-  /**
-   * {@inheritdoc}
-   */
-  protected function tearDown() {
-    parent::tearDown();
-    static::$functions = [];
-  }
-
   /**
    * Tests getting legacy theme function registry data defined by a module.
    *

Run the test locally - it's fine.

I also fixed the following coding standards:

FILE: ...lt.dev/core/tests/Drupal/Tests/Core/Theme/RegistryLegacyTest.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
   3 | ERROR | [x] Namespaced classes, interfaces and traits should
     |       |     not begin with a file doc comment
 146 | ERROR | [x] Expected 1 space after FUNCTION keyword; 0 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 71ms; Memory: 8MB


FILE: ...upal8alt.dev/core/tests/Drupal/Tests/Core/Theme/RegistryTest.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
   3 | ERROR | [x] Namespaced classes, interfaces and traits should
     |       |     not begin with a file doc comment
 485 | ERROR | [x] Expected 1 space after FUNCTION keyword; 0 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------