run-tests.sh --module is broken

1. +++ b/core/modules/simpletest/simpletest.module
   @@ -434,7 +434,7 @@ function simpletest_log_read($test_id, $prefix, $test_class, $during_test = FALS
     */
   -function simpletest_test_get_all() {
   +function simpletest_test_get_all($module = NULL) {
   

   Needs a docblock update. Same for multiple functions below, looks like one of them even mentioned non-existing arguments...

   Also, this function has a persistent and static cache, you need to consider $module there as well, otherwise it won't work correctly. You'll always only get the module of the first cache clear... Alternatively don't bother caching if $module isn't empty, probably not worth it?

2. +++ b/core/modules/simpletest/simpletest.module
   @@ -453,6 +453,11 @@ function simpletest_test_get_all() {
   +      if (isset($module)) {
   +        $all_data = array(
   +          $module => $all_data[$module],
   +        );
   +      }
   

   Why isset, wouldn't if ($mnodule) be enough?

   Took me a few seconds to understand what this is doing, a quick comment might be useful?

Confirmed that the patch works, php ./core/scripts/run-tests.sh --module node right now results in RuntimeException: Sub-class must implement the getInfo method! in Drupal\simpletest\TestBase::getInfo(), with the patch, it successfully identifies all tests in node.module and starts executing them.

However, I do have some nitpicks, you won't like me for this...

1. +++ b/core/modules/simpletest/simpletest.module
   @@ -419,6 +419,9 @@ function simpletest_log_read($test_id, $prefix, $test_class, $during_test = FALS
   + * @param string $module
   + *   Name of a module. If set then only tests belonging to this module is
   + *   returned.
   

   *are* returned, I think?

   Also needs an empty line between the @param's and @return.

2. +++ b/core/modules/simpletest/simpletest.module
   @@ -724,15 +736,21 @@ function simpletest_library_info() {
    /**
     * Get PHPUnit Classes
     *
   - * @param bool $name_only
   - *  If TRUE, returns a flat array of class names only.
   + * @param string $module
   + *   Name of a module. If set then only tests belonging to this module is
   + *   returned.
     */
   -function simpletest_phpunit_get_available_tests() {
   +function simpletest_phpunit_get_available_tests($module = NULL) {
   

   I know you didn't add this (@msonnabaum very likely did, so kick him, not me :)), but it's missing @return (copy from another one) and the initial line is wrong in multiple lines, should be "Gets PHPUnit classes." IMHO, but I'm OK with that not being fixed here.

3. +++ b/core/scripts/run-tests.sh
   @@ -341,9 +341,9 @@ function simpletest_script_init($server_software) {
   -function simpletest_script_get_all_tests() {
   -  $tests = simpletest_test_get_all();
   -  $tests['PHPUnit'] = simpletest_phpunit_get_available_tests();
   

   Unfortunately another function without updated docblock.

No reason to won't fix, let's just tag it for a novice re-roll...

I'm not sure I've re-rolled this correctly. I was confused by list item 3 in comment #4.

+++ b/core/scripts/run-tests.sh
@@ -341,9 +341,9 @@ function simpletest_script_init($server_software) {
-function simpletest_script_get_all_tests() {
-  $tests = simpletest_test_get_all();
-  $tests['PHPUnit'] = simpletest_phpunit_get_available_tests();

Unfortunately another function without updated docblock.

Let me know what more needs to be done

It was about simpletest_script_get_all_tests() missing the doc for the new $module param, and it looks great now.

8: 2132441_8.patch queued for re-testing.

8: 2132441_8.patch queued for re-testing.

Agreed, looks great now, thanks! (The only thing is the still missing "." at the end of one of those function descriptions, but that was already missing before and could easily be added before commit.

Only issue is we don't have test coverage for this regression, so we may re-introduce it once again. But in talking to Tim, he seemed to think it was basically impossible to pull this off in run-tests.sh. So...

Committed and pushed to 8.x, including the missing "." ;)