run-tests.sh ignores final classes

#2551981: Add --directory option to run-tests.sh test discovery introduced this.

Re-discovered while working on #3081705: Make DrupalCI discover and run JSON:API Hypermedia's tests. Incredible that not a single person touched this issue in almost two years :O

Should we add a final test for this, to test if this works? I don't see a way to verify this automatically but we can at least check manually in Drupal CI results

#7++ — yes, but only if this is acceptable at all. I wonder if we explicitly do not want to allow for final test classes, and that's perhaps why this was never fixed?

I think we should allow for final tests because that follows the principle of least surprise. If you as contrib or custom declare a test as final because well why not... and the test system doesn't discover it even though you can run it by pointing at the file directly that will take an age to debug. Adding a test with final is not enough to test this though. We need to test that run-tests.sh can discover it and I'm not sure we have that level of run-tests.sh testing anywhere. @mile23 has been trying to make run-tests.sh testable but it's a slow process because it's currently untested so changes are risky and complex.

I think a way forward is manual testing - or uploading a test-only path with a final test that's not discovered and one with the fix where it is discovered but still commit the same patch as in #5 as the tests require manually looking at what tests have run.

This is the code path that applies to run-tests.sh --directory, which is used by drupalci for contrib. https://git.drupalcode.org/project/drupal/blob/8.8.x/core/scripts/run-te...

The same thing applies to --file. We still allow multiple test classes per file, BTW. That regex is supposed to also exclude abstract classes, but the regex is different. https://git.drupalcode.org/project/drupal/blob/8.8.x/core/scripts/run-te...

One semi-sustainable solution here is to add another function named something like simpletest_script_get_test_list_from_file() so we can at least have the regexes in the same place. That includes the one that discovers the namespace.

The tricky part about testing this core change with drupalci is that drupalci won't use --directory or --file for core, so even if you add a final test to core as a patch, drupalci will (should?) still discover it.

In an ideal world, we'd have a tiny utility class in Drupal\Core\Tests\ that knows how to do this, instead of adding simpletest_script_get_test_list_from_file(). Then we could build vfsStream mocks to our heart's content and check its behavior. That makes for easier patch review and also easier maintenance down the road.

Here's a patch that refactors --file and --directory to use a utility class.

This will fail the final class unit test, because the regex in #5 isn't quite right. This uses the old regex.

And phail on the follow-through. Let's try that again.

Todo: Mark this as @internal. :-)

Fixes regex:

@^\s*(?!abstract\s+)(?:final\s+|\s*)class ([^ ]+)@m

Adds another test case.

CS.

Marks this thing as @internal so we can refactor later as needed.

Much better than my rough solution! :)

+++ b/core/tests/Drupal/Tests/Core/Test/RunTests/TestFileParserTest.php
@@ -68,10 +81,9 @@ public function testParseContents($expected, $contents) {
+    // This test.

🤔 This is an odd comment.

If it weren't for this, I'd RTBC :)

Removed offending comment.

Want to see something funny? #2871289-11: Make run-tests.sh --directory testable

+++ b/core/lib/Drupal/Core/Test/RunTests/TestFileParser.php
@@ -0,0 +1,61 @@
+namespace Drupal\Core\Test\RunTests;

Hm, this is a new namespace. I … am not sure about this. But I don't feel strongly about it either. I'll see what a core committer thinks — in the end it doesn't matter much, especially because the sole class here is @internal anyway.

Also using that namespace in #3074433: Decouple run-tests.sh from the simpletest module

If we have another class to add to the new namespace elsewhere, I think it's OK to add here. We can always move things around later (famous last words etc. but in theory at least).

Committed 73972cc and pushed to 8.8.x. Thanks!

Wim Leers asked in slack if we could backport this - I think it's OK for backport, but it doesn't cherry-pick, so will need a re-roll against 8.7.x

Rerolled for 8.7.x

Committed 616d769 and pushed to 8.7.x. Thanks!