Spin-off from Testbot/PIFR issue: #1674290: PSR-0 tests in contrib modules are not found

Problem

  • run-tests.sh does not allow to run all tests of a certain module. (see PIFR issue linked above)
  • run-tests.sh unconditionally retrieves all test case classes available in a Drupal site, even when not needed.

Goal

  • run-tests.sh --module node
  • Minimize dependencies on an existing/installed Drupal [parent] site. (to be continued in follow-up issues)

Related issues

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Issue tags: +Needs backport to D7
sun’s picture

Yes. This patch removes all sanity checks for the passed script parameters. That's no mistake.

A shell script should fail prominently and visibly. There is no point at all in hiding errors.

sun’s picture

This patch is RTBC from my perspective.

I've manually tested the possible arguments already and there's no regression. The new --module argument also works as expected.

Niklas Fiekas’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/scripts/run-tests.shundefined
@@ -489,44 +486,65 @@ function simpletest_script_cleanup($test_id, $test_class, $exitcode) {
+        if (preg_match('@^namespace ([^ ;]+)@m', $content, $matches)) {
+          $namespace = $matches[1];
+        }
+        // Extract all class names.
+        // Abstract classes are excluded on purpose.
+        preg_match_all('@^class ([^ ]+)@m', $content, $matches);

Maybe (if we ever encounter a problem) we can do better than that, by actually parsing the file (http://de2.php.net/manual/en/function.token-get-all.php), but it should be solid enough generally.

+++ b/core/scripts/run-tests.shundefined
@@ -542,7 +560,7 @@ function simpletest_script_get_test_list() {
-  global $args, $all_tests, $test_list, $results_map;
+  global $args, $test_list, $results_map;

I don't get why this had $all_tests in the first place. So yeah.

Looks good to me. Thanks sun!

sun’s picture

The --file option actually looks obsolete to me, and I'd rather try to remove it entirely in a later, separate step. Right now, PIFR actually solely uses that option only, so removing it will require some additional orchestration.

Niklas Fiekas’s picture

Right. With PSR-0 there is not much flexibility for the filename while holding on namespace + class name.

Dries’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed to 8.x. Thanks. Moving to 7.x.

sun’s picture

Version: 7.x-dev » 8.x-dev
Status: Patch (to be ported) » Active

#1700682-47: run-tests.sh attempts to run abstract/base test classes revealed:

The test is here: http://qa.drupal.org/pifr/test/310618 (Chessboard Filter is a module in the Chessboard project: drupal.org/project/chessboard)

But how about a reasoning like this: my test class cannot be auto-loaded here unless the namespace is registered and that won't happen until the module is enabled. The module will not be enabled until the test class object is alive.
So the code block that deals with the --file option needs to call simpletest_classloader_register() just like simpletest_test_get_all() does.

EDIT: Though it does not make much sense to register disabled modules namespaces when you're not going to run all tests but just the ones from the explicitely passed (PSR-0) files.

I object to calling simpletest_classloader_register() though. We need to inline that function instead. The ultimate goal is to get rid of the simpletest.module dependency.

sun’s picture

Status: Active » Needs review
FileSize
509 bytes

Attached patch is the quick-fix for that bug.

jthorson’s picture

Views 8.x-3.x test with the above patch fails with the following:

ReflectionException: Class Drupal\views\Tests\Style\PluginStyleJumpMenuTest does not exist in ReflectionClass->__construct() (line 557 of /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh).

jthorson’s picture

Status: Needs review » Needs work
root@scratchtestbot-mysql:/var/lib/drupaltestbot/sites/default/files/checkout# /usr/bin/php ./core/scripts/run-tests.sh --php /usr/bin/php --url 'http://drupaltestbot659-mysql/checkout' --file sites/all/modules/chessboard/chessboard_filter/lib/Drupal/chessboard_filter/Tests/ChessboardFilterTest.php 
ReflectionException: Class Drupal\chessboard_filter\Tests\ChessboardFilterTest does not exist in ReflectionClass->__construct() (line 557 of /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh).

And, at risk of polluting this thread even further, so does Chessboard.

xjm’s picture

Confirmed this also does not resolve the problem using the script manually locally.

sun’s picture

Status: Needs work » Needs review
FileSize
2.54 KB

Due to the sub-processes being spawned by run-tests.sh, this was a bit more complex to figure out.

Should even give a nice performance boost, at least for contributed projects. At least for me, the total time for a single test case decreased by 2 seconds.

Status: Needs review » Needs work

The last submitted patch, drupal8.run-tests-disabled.13.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
2.61 KB

Still doesn't work correctly, but wanted to provide the latest code.

sun’s picture

Assigned: sun » Unassigned
Issue summary: View changes
Status: Needs review » Fixed
Issue tags: -Needs backport to D7

The follow-up patch seems to be obsolete. In any case, if there's anything that needs a follow-up, let's create a separate issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.