Add --directory option to run-tests.sh test discovery

How would this address the issue with ubercart, where we want to run all the tests included with each individual submodule, which are all in separate directories? From what I can tell, --module should be fixed to properly handle that situation.

1. +++ b/core/scripts/run-tests.sh
   @@ -836,6 +839,60 @@ function simpletest_script_get_test_list() {
   +      if ($args['directory'][0] === '/') {
   

   I personally find array access to strings very unreadable. I'd go with string functions to get the first character instead.

2. +++ b/core/scripts/run-tests.sh
   @@ -836,6 +839,60 @@ function simpletest_script_get_test_list() {
   +        // '/Tests/' can be contained anywhere in the file's path (there can be
   

   We're using stripos() for the check, which is case-insensitive, but we should probably clarify that in the comment, so '/Tests/' or '/tests/' can be contained...

3. +++ b/core/scripts/run-tests.sh
   @@ -836,6 +839,60 @@ function simpletest_script_get_test_list() {
   +        //   ./lib/Drupal/foo/Tests/Bar/Baz.php
   +        //   ./foo/src/Tests/Bar/Baz.php
   +        //   ./foo/tests/Drupal/foo/Tests/FooTest.php
   +        //   ./foo/tests/src/FooTest.php
   

   I like these examples. Very helpful!

4. +++ b/core/scripts/run-tests.sh
   @@ -836,6 +839,60 @@ function simpletest_script_get_test_list() {
   +        // Strip the drupal root directory and trailing slash off the URI
   

   We're not stripping the trailing slash of the URI, but the separator slash between the root path and the rest (I have no fancy name for that).

5. +++ b/core/scripts/run-tests.sh
   @@ -836,6 +839,60 @@ function simpletest_script_get_test_list() {
   +        if (stripos($filename, '/Tests/')) {
   

   I would make this check a little more explicit, to clarify we're looking for a positive integer.

6. +++ b/core/scripts/run-tests.sh
   @@ -836,6 +839,60 @@ function simpletest_script_get_test_list() {
   +        $content = file_get_contents($file);
   +        // Extract a potential namespace.
   +        $namespace = FALSE;
   +        if (preg_match('@^namespace ([^ ;]+)@m', $content, $matches)) {
   +          $namespace = $matches[1];
   +        }
   

   Maybe document we extract the namespace using string manipulation because this was copied verbatim from old code?

   Actually, since we load the class later anyway to check if it extends any of the base classes, we can easily use reflection to replace the regular expressions.

7. +++ b/core/scripts/run-tests.sh
   @@ -836,6 +839,60 @@ function simpletest_script_get_test_list() {
   +        // Abstract classes are excluded on purpose.
   

   Where do we exclude abstract classes?

8. +++ b/core/scripts/run-tests.sh
   @@ -836,6 +839,60 @@ function simpletest_script_get_test_list() {
   +          foreach ($matches[1] as $class_name) {
   

   PSR-0/4 prescribe the (primary) class in any file must have the same name as the file, minus the extension, so we can probably make this simpler, combined with my earlier comment about reflection?

Just a couple of thoughts while reviewing the patch in #2. I do not feel comfortable RTBC because of my lack of experience with PIFR code base.

1. +++ b/core/scripts/run-tests.sh
   @@ -836,6 +839,60 @@ function simpletest_script_get_test_list() {
   +      if ($args['directory'][0] === '/') {
   +        $directory = $args['directory'];
   +      }
   +      else {
   +        $directory = DRUPAL_ROOT . "/" . $args['directory'];
   +      }
   

   I'm not familiar with PIFR's argument handling, but this seems strange.

   Is the directory argument a string or an array? When is it in array?

2. +++ b/core/scripts/run-tests.sh
   @@ -836,6 +839,60 @@ function simpletest_script_get_test_list() {
   +        $filename = substr($file->uri, strlen(DRUPAL_ROOT)+1);
   

   What other API functions are available?

   • file_uri_target() returns the base path and strips off the leading and trailing slashes.
   • drupal_realpath() would get a file uri's full file path, if it exists.

   That may not be available when porting to Drupal 6 though.

3. +++ b/core/scripts/run-tests.sh
   @@ -836,6 +839,60 @@ function simpletest_script_get_test_list() {
   +        $content = file_get_contents($file);
   

   I'm not a fan of this existing PIFR code for getting namespaces, but it seems like the easiest to port between Drupal 6, 7, and 8.

   In Drupal 8 it would be possible to check the autoloader. There may be an API call for that if we had access to \Drupal.

   In Drupal 7, there's local phpunit and simpletests the latter should be defined in the info file and the former in phpunit.xml(.dist). There's probably no better solution for Drupal 6 or Drupal 7 code.

Adding a related issue that is listed as an infrastructure blocker to d8.

> I personally find array access to strings very unreadable

It's not array access, it's merely a string offset. There has been many wars wrought over [] vs [} to make this more explicit, it's [] and we will live with it.

> We're using stripos() for the check, which is case-insensitive, but we should probably clarify that in the comment, so '/Tests/' or '/tests/' can be contained...

Noone, ever reads this script.

> Strip the drupal root directory and trailing slash

That's what we strip, the drupal root directory + the trailing slash of that directory.

> I would make this check a little more explicit,

I wouldn't :) it's explicit enough: stripos can return FALSE or a nonnegative integer. We do not need to document PHP especially in a script that noone ever looks into. I am very much for documentation but that's for things people need to call or stumble on when debugging. This is not one.

> Where do we exclude abstract classes?

preg_match_all('@^class right there: abstract classes do not start with the word class at least not if they adhere to any coding standards.

> PSR-0/4 prescribe the (primary) class in any file must have the same name as the file

Uh, if this needs to be backported then classes won't be PSR... same about reflection, when we backport to D7 then you'll hit a fatal when trying to read namespaces from reflection in a PHP version which doesn't have namespaces.

> Is the directory argument a string or an array? When is it in array?

String. $args['directory'] is a string.

Committed/pushed to 8.0.x, thanks!

Heres the D7 backport patch. The testbots will likely apply this patch to all d7 tests until this makes it into a d7 release.

php5.4 array syntax fixed.

Lets try that again, this time with more fury.

This looks like it will find Drupal 8 tests only, but not standard Drupal 7 .test files?

Also, maybe we can find a way to reuse the code in simpletest_test_get_all() here. It's trying to do a similar thing and it would be unfortunate to have to duplicate a lot of it...

Yep, Im in the midst of getting this working - its looking for the wrong class currently, and I miscopied the directory => NULL, to directory => FALSE

simpletest_test_get_all() in d7 only retrieves all tests (it wasn't until d8 that the concept of passing in a $module is possible. It may be that *that* is what needs a backport here).

I'll post another patch soon.

Okay, I've been testing this patch out and its finding tests properly.

I've added a couple of small things, namely excluding tpl.php and api.php files from the recursive search.

This will find any tests named .test, or any php file under a /tests/ directory, as long as they extend \DrupalTestCase. This may or may not be true for contrib, but so far it has found the right tests (tried views/ctools/examples).

The comments probably need work, but Im going to put this into the simpletestlegacyd7 as a temporary patch so we can exercise it more on the testbots with other code.

@David_Rothstein: thanks for the pointer to simpletest_test_get_all() - after looking deeper into it, I realized that it does indeed already have all the logic necessary to find all the tests, and that all we needed to do is mimic what lives under the --file option to compare the files we find vs the tests that exist to get the test list.

Here's a patch that works much better for everything I've been testing against.

Small nitpick review.

+++ b/scripts/run-tests.sh
@@ -451,6 +454,51 @@ function simpletest_script_get_test_list() {
+        } else if (stripos($filename, '.test')){

Missing space after control statement and left bracket.

And else if should be elseif on its own line.

RTBC! Looks great to me!

We'll have to coordinate this close;y with Drupal CI to avoid failing tests e.g. when git apply --index fails.

Lets test that out by testing another PHP version.

Yeah, as I thought. Testbots should use patch --dry-run before the git apply, which is more forgiving if a patch already is applied ...

ah yeah, drupalci applies this patch to run the d7 tests. I'll investigate the --dry-run - is the expected behavior that "in the event a patch is already applied, continue without applying the patch" ?

#29> No, what I meant is:

patch --dry-run < 'patch.diff' && git apply ...

Or alternatively:

git apply --check 'patch.diff' && git apply 'patch.diff'

This is blocking other work to be tested.

I added an || true to the "cd /var/www/html && git apply ./2551981-21-add-directory-option-to-run-tests.patch || true", as even the dry run was returning a non-zero error code if it didn't apply. We don't really need to abort processing if that patch does not apply, and it looks like we're actually getting a test run in #26. You'd be free to commit this patch without affecting drupalci.

G-G-G-Green!

Great! Back to RTBC!

1. +++ b/scripts/run-tests.sh
   @@ -190,6 +192,7 @@ function simpletest_script_parse_args() {
        'file' => FALSE,
   +    'directory' => NULL,
   

   I check this line and we use '' elsewhere.

   Will fix on commit.

2. +++ b/scripts/run-tests.sh
   @@ -451,6 +454,51 @@ function simpletest_script_get_test_list() {
   +      $ignore = array('nomask' => '/vendor|\.tpl\.php|\.api\.php/');
   

   Im theory we do not support /vendor, however even with just phpunit, you could have an additional vendor/ directory in your tests/ directory, so this gets a +1 from me and as its always an absolute path, there will also be a match.

3. +++ b/scripts/run-tests.sh
   @@ -451,6 +454,51 @@ function simpletest_script_get_test_list() {
   +      if ($args['directory'][0] === '/') {
   

   This is non-performance sensitive code, so we prefer:

   strpos($args['directory'], '/') === 0

   In this case it would always work as we have a string and know it is non-empty though ...

   --

   Will be fixed on commit.

4. +++ b/scripts/run-tests.sh
   @@ -451,6 +454,51 @@ function simpletest_script_get_test_list() {
   +        if (stripos($filename, '/Tests/')) {
   +          $files[drupal_realpath($filename)] = 1;
   +        } else if (stripos($filename, '.test')){
   +          $files[drupal_realpath($filename)] = 1;
   

   The stripos() should check !== FALSE as they can in theory return 0.

   However again as we deal with an absolute path, that won't be a problem in practice.

   --

   Will fix on commit anyway, after I checked the code base some more for occurrences of this in Drupal 7.

5. +++ b/scripts/run-tests.sh
   @@ -451,6 +454,51 @@ function simpletest_script_get_test_list() {
   +      // Check for valid class names.
   +      foreach ($all_tests as $class_name) {
   

   This code is duplicated below, I think this should be consolidated and this method just setting up test_names and --file option.

   --

   Will work on that.

Reviewed again in depth and found some things that need work and can be nicer.

I decided to only make the minimal change as the 8.x commit also has stripos(), so if we fix that, we need to fix it in both versions.

And again portability of code (same structure as in D8) beats making it perfect.

If someone volunteers, we can open a follow-up D8 + D7 issue though.

Given DrupalCI already uses this patch and given my extensive review above, I feel good to commit this.

Committed and pushed to 7.x! Thank you all very much!

Fixed on commit:

diff --git a/scripts/run-tests.sh b/scripts/run-tests.sh
index 0e5ba28..786ef27 100755
--- a/scripts/run-tests.sh
+++ b/scripts/run-tests.sh
@@ -192,7 +192,7 @@ function simpletest_script_parse_args() {
     'all' => FALSE,
     'class' => FALSE,
     'file' => FALSE,
-    'directory' => NULL,
+    'directory' => '',
     'color' => FALSE,
     'verbose' => FALSE,
     'test_names' => array(),

as the empty string evaluates to FALSE as well and NULL is not used anywhere else in the parameters.

Added a new tag for issues that need some love in D7 and D8.

It could probably use some cleanup for the code comments too; they seem very Drupal 8-specific since they don't refer to the main type of Drupal 7 test files (*.test).

Also per #18 it really seems to me like it would be possible (and preferable) to reuse https://api.drupal.org/api/drupal/modules!simpletest!simpletest.module/f... here..... i.e. add an optional $directory parameter to that function and then call it. Is there some reason that wouldn't work? (That function is already supposed to be the definitive determination of what tests exist, I think. And it's already called from elsewhere in run-tests.sh.) Perhaps this would make sense as a followup for Drupal 8 also.

It's not very obvious from the patch, but this *is* reusing simpletest_test_get_all().

There is a global variable set in lines 49-54 that contains the list of all tests.

// Load SimpleTest files.
$groups = simpletest_test_get_all();
$all_tests = array();
foreach ($groups as $group => $tests) {
  $all_tests = array_merge($all_tests, array_keys($tests));
}

The patch loops over that global $all_tests starting in line 495 to validate whether or not the files we have discovered are a member of that list of all tests:

+++ b/scripts/run-tests.sh
@@ -451,6 +454,51 @@ function simpletest_script_get_test_list() {
+      // Check for valid class names.
+      foreach ($all_tests as $class_name) {
+        $refclass = new ReflectionClass($class_name);
+        $classfile = $refclass->getFileName();
+        if (isset($files[$classfile])) {
+          $test_list[] = $class_name;
+        }
+      }

So the other issue with altering the function signature to add an optional directory to simpletest_test_get_all is that the function signature in d8 takes in an *extension*, not a *directory*.

The --directory option is there to allow for running tests that exist in an entire *project*, not limited to individual extensions that come with that project (which is what the --module flag already does).

So, while we might be able to embed this functionality into simpletest_test_get_all, we'd be deviating from the direction that d8 has already gone.

An alternative would be to add another function into simpletest that has this functionality wrappering simpletest_test_get_all, and have run-tests.sh do less logic, but I dont know that we necessarily need to do that, unless we have an equivalent in d8 as well.

We can (both in D7 and D8) roughly do:

  if ($args['directory']) {
    // Code with the file_scan_directory().
    $args['test_names'] = $files
    $args['file'] = TRUE;
  }
  if ($args['file']) {
    // Already has support for filtering tests by files
  }

because the code is identical.

However if we want to do any cleanup, I suggest we start with D8, then back port it.