run-tests.sh attempts to run abstract/base test classes

Don't know the internals of the $test_classes array but my feeling says this should be good to go.

I think this is rtbc to.

It is unfortunate that we have to load all test classes in order to check whether they have a ::getInfo() method, but I do not see another way around this.

What about reflection? Not sure it matters though, run-tests.sh is definitely not the bottleneck in running tests.

? This still needs to load every test. Catch wanted to use reflection when building the list.

Ok lets commit #10 so views D8 tests can run again

Note: if we commit #10 then soon-ish #1698108: Update Drupal's dependencies we can use the tokenizer code I wrote for Doctrine to replace the ReflectionClass with a tokenizer-based approach so that you do not need to include all the classes (yet you need to read them, yes). It was meant for annotations but seems useful here as well. See an example here on how this is done.

Testbot cannot instantiate an abstract class, because they are abstract. PHP will not allow it. The problems we had in Views were solved by properly making base classes abstract.

I think this should be closed, but leaving open for discussion.

sun when you make the base classes abstract the test runner won't attempt to do it.

I think checking for getInfo is incorrect. This makes more sense to me.

However, I don't think this is actually legitimate, core and contrib are full of abstract classes that aren't blowing up on testbot.

Sorry for double post. :)

I should note that I'm particularly concerned about automatically skipping any getInfo()-less class -- I think this would mean (as Tim has stated) that a broken test without a getInfo() method would silently never be run, which is a concern, as no one will typically scan through all 450 core tests classes to ensure one has run. However, I'm going to actually get some manual testing with this issue to see what's happening.

Threw together a sandbox. Haven't actually tested with it yet.
http://drupal.org/sandbox/xjm/1705218

Base classes should be abstract. Tim mentioned yesterday that 25 in core currently are not, and that #1674290: PSR-0 tests in contrib modules are not found was caused by a views bug.

Sorry, which patch are you RTBCing? I'd much rather just do this.

Fixed my regex, I accidentally marked NodeAccessBaseTableTest as abstract :)

Call to undefined function Drupal\system\Tests\Upgrade\language_negotiation_include()

I think I've seen that before, can't remember where.

Core is broken cause of a missing contact.settings.yml file. And missing config files lead to this error in upgrade tests.

#27: drupal-1700682-27.patch queued for re-testing.

If the classes are abstract, run-tests.sh will happily ignore them. Because PHP won't let it do anything but ignore it.

The only error run-tests.sh has ever given on this topic is when a non-abstract class with a missing getInfo is run.

Those errors should continue to cause fatals, because that is an invalid test class.

Compromise. Combines #18 and #27, without the duplicated hunk in run-tests.sh.

This looks good but the patch no longer applies. Asking for a re-roll:

vortex:drupal dries$ git apply ../f.patch 
error: patch failed: core/modules/field_ui/lib/Drupal/field_ui/Tests/FieldUiTestBase.php:12
error: core/modules/field_ui/lib/Drupal/field_ui/Tests/FieldUiTestBase.php: patch does not apply

Not sure why this is 'major'. Should probably be 'normal', unless explained otherwise.

Conflicted with #1380958: Replace $modules list for WebTestBase::setUp() with ::$modules class properties. It will also conflict with the list of patches in #1711070: Convert tests to use ::$modules property instead of parent::setUp($modules), so if it's okay with sun, I'm going to postpone this.

Yeah, and those other issues have to all be rerolled it turns out for doc blocks, proceed!

Committed to 8.x. Thanks!

It appears this killed d.o. contrib testing. Would that be a --file option issue or a PIFR issue?

Output: [ReflectionException: Class Drupal\chessboard_filter\Tests\ChessboardFilterTest does not exist in ReflectionClass->__construct() (line 554 of /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh).].

On a side note: for the --file option the abstract classes were already being excluded by regex.

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.

This same error shows up in the log for Views on QA:

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

I confirmed checking out the commit before the one in this issue allows views tests to be run again. It is not possible to run any views tests with this patch.

Can we revert this commit until we sort the issue out?

Actually, rather than reverting it, let's just revert the change to run-tests.sh. The rest is legit.

And next time let's please test the script manually before committing changes to it. :)

I did test this manually, and it worked fine.

I think this might be a PHP version issue?
I'm using 5.3.13, can someone test with 5.3.3?

I have 5.3.6 and get the same error as on the bot. Checking the bot's php version in #1635240-4: Disregard -- patch testing issue ☢.

So I don't think this is actually related to the PHP version; just to what modules are enabled in the parent environment. The classes are not detected when (in our case) views and ctools are disabled.

[lorentz:vdc | Mon 20:38:09] $ php core/scripts/run-tests.sh --url 'http://localhost:8888/vdc'  --file sites/all/modules/views/lib/Drupal/views/Tests/Style/PluginStyleJumpMenuTest.php

Drupal test run
---------------

Tests to be run:
 - Jump menu (Drupal\views\Tests\Style\PluginStyleJumpMenuTest)

Test run started:
 Monday, August 6, 2012 - 20:38

Test summary
------------

Jump menu 6 passes, 0 fails, and 0 exceptions

Test run duration: 4 sec

[lorentz:vdc | Mon 20:38:46] $ drush dis views -y
The following extensions will be disabled: views
Do you really want to continue? (y/n): y
views was disabled successfully.                                     [ok]
[lorentz:vdc | Mon 20:39:08] $ drush dis ctools -y
The following extensions will be disabled: ctools
Do you really want to continue? (y/n): y
ctools was disabled successfully.                                    [ok]
[lorentz:vdc | Mon 20:39:13] $ php core/scripts/run-tests.sh --url 'http://localhost:8888/vdc'  --file sites/all/modules/views/lib/Drupal/views/Tests/Style/PluginStyleJumpMenuTest.php
ReflectionException: Class Drupal\views\Tests\Style\PluginStyleJumpMenuTest does not exist in ReflectionClass->__construct() (line 554 of /Applications/MAMP/htdocs/vdc/core/scripts/run-tests.sh).
[lorentz:vdc | Mon 20:41:26] $ git apply run-tests-1700682-52.patch
[lorentz:vdc | Mon 20:41:29] $ php core/scripts/run-tests.sh --url 'http://localhost:8888/vdc'  --file sites/all/modules/views/lib/Drupal/views/Tests/Style/PluginStyleJumpMenuTest.php

Drupal test run
---------------

Tests to be run:
 -  (Drupal\views\Tests\Style\PluginStyleJumpMenuTest)

Test run started:
 Monday, August 6, 2012 - 20:41

Test summary
------------

Jump menu 6 passes, 0 fails, and 0 exceptions

Test run duration: 4 sec

[lorentz:vdc | Mon 20:41:37] $ 

Earlier fail was a bot goof.

Steps to test manually:

1. Install 8.x core and enable simpletest.
2. Download a contributed module with an 8.x test suite (and any dependencies) into the copy of 8.x, but do not enable the module.
3. Use the run-tests.sh command to run a test from the module, e.g.:
   

   php core/scripts/run-tests.sh --url 'http://localhost:8888/vdc'  --file sites/all/modules/views/lib/Drupal/views/Tests/Style/PluginStyleJumpMenuTest.php
   

   You should get an error like:

   ReflectionException: Class Drupal\views\Tests\Style\PluginStyleJumpMenuTest does not exist in ReflectionClass->__construct() (line 554 of /Applications/MAMP/htdocs/vdc/core/scripts/run-tests.sh).
   

4. Enable the module and its dependencies and re-run the test. It should now be executed.
5. Re-disable the module and apply the patch from #52, which rolls back the committed change from #41. The test should now be executed even with the module disabled in the parent environment.

Can we clarify what, exactly, the purpose of the hunk that the patch in #52 reverts is? Steps to test manually?

• The issue with Views (described in #13) was resolved in http://drupalcode.org/project/views.git/commit/2e90997b053877fe9f1900b0d...
• The issue with inline was (apparently) unrelated, per #6.

Verified via scratchtestbot that #52 does get the tests running for Views and Ctools, but Chessboard still failed with "Class not found" errors. Core passed during my manual tests as well.

Still, re-enabling testing for 'some' modules is slightly better than the current 'no' modules; so would agree with rolling back the patch in #52 for now, with the caveat that this issue remain open until i) a new patch is provided which addresses the root issue for both core and contrib, or ii) the issues with the Chessboard failures can be explained and attributed to an unrelated issue.

Edit: Chessboard is running and passing, so must have been an issue with my testing environment. Also, based on http://qa.drupal.org/pifr/test/312233, it appears that the abstract classes are being ignored properly ... so retracting the caveats above.

Thanks a lot for all of the hard work on the testing. I've committed and pushed #52 to 8.x in order to re-enable contributed module testing. Marking needs work.

I'd also love to know the answer to xjm's queries in #60, since reading through this issue was pretty confusing and I'm still not completely sure what's being fixed here. And, assuming this is fixing a legit bug, it seems like the kind of thing we ought to add test coverage for, if we can.

So:
http://qa.drupal.org/pifr/test/312233

Tests to be run:
 -  (Drupal\broken_tests\Tests\NonAbstractChildTest)
 -  (Drupal\broken_tests\Tests\GetInfolessTest)
 -  (Drupal\broken_tests\Tests\NonAbstractWithMethodChildTest)
 -  (Drupal\broken_tests\Tests\NonAbstractTestBase)
 -  (Drupal\broken_tests\Tests\AbstractWithMethodChildTest)
 -  (Drupal\broken_tests\Tests\AbstractChildTest)
 -  (Drupal\broken_tests\Tests\NonAbstractWithMethodTestBase)

The module includes the following classes:

AbstractChildTest
NonAbstractChildTest
AbstractTestBase
NonAbstractTestBase
AbstractWithMethodChildTest
NonAbstractWithMethodChildTest
AbstractWithMethodTestBase
NonAbstractWithMethodTestBase
GetInfolessTest

As you can see in the results, it attempts to run non-abstract, broken "base" classes NonAbstractWithMethodTestBase and NonAbstractTestBase, as well as all the child classes; however, it does not attempt to run AbstractWithMethodTestBase nor AbstractTestBase. This is with #52 already applied.

So, making base classes abstract (as they should be anyway) resolves the issue as far as I can tell. It did in views.

Oops, xpost (though as mentioned in IRC, I'm not sure that this is testable programmatically).

Note on above syntax examples: for me, back slashes are stripped from class names specified with flag --class unless the class name is enclosed in quotation marks.