The SimpleTests should be PSR-0. Starting with Tracker because why not... The namespace Drupal\Module\Tracker is still under debate over at #1290658: Move all module-provided classes to PHP namespaces (PSR-0 or similar), and autoload them without the registry [policy, no patch], but I'd rather do what's important instead. Switching namespaces is simple. There are two tasks that would help here:

  1. Kill the need of tracker_simpletest_alter()
  2. Kill the need of tracker_init() via #1467126: PSR-0 namespace auto registration for modules
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RobLoach’s picture

Issue tags: +PSR-0

.

RobLoach’s picture

FileSize
1.95 KB

Drupal\Tracker\Test\TrackerTest

sun’s picture

So yeah. Reasoning for #2 is:

Symfony people are arguing that tests shouldn't be part of an extension's namespace at all. Symfony does not provide tests with their components. As such, Symfony components are not self-contained, they have /lib, /vendor, and /tests -- whereas components are intermixed with others in /lib and /vendor, and all tests for all components pile up in /tests. The most I was able to distill from Symfony developer feedback is that Symfony simply separates tests entirely, because they didn't want to look into the full DI being required for their test cases. That's a bug on its own. Therefore, we shouldn't care what Symfony does or not.

Drupal extensions, however, are self-contained in every way. A module should provide its own tests.

Thus, Drupal\Tracker\Tests sounds most reasonable.

(whereas Drupal\Tracker denotes Tracker module's namespace, and .\Tests are the tests in that namespace)

However, plural vs. singular bites me here.

So it should be Drupal\Tracker\Test to be consistent with everything else.

Hence, e.g., Drupal\Tracker\Test\NodeTestCase

Each test case class is in its own file. (we should write a script for this, every single contrib developer will have to do the identical conversion).

pounard’s picture

I would totally love to have an external DrupalTest namespace, Zend does this and it's fine. They actually does something like this (from my memory):

library/Zend/ # Zend namespace
tests/Zend/ #ZendTest namespace
tests/_autoload.php # Custom autoloader for tests
tests/Bootstrap.php # PHP environment bootstrapping
tests/phpunit.xml # PHPUnit configuration
tests/runtests.sh # Runner

Which gives us test class names as such:

Zend\Acl becomes ZendTest\Acl for testing Zend\Acl feature

So the tests are not fully PSR-0 compliant due to the Zend folder name under tests/ but it is quite straightforward once you know it. The thing is, for tests, we need an autoloader which is not initialized by core (it doesn't make sense to bootstrap core for tests).

As they use PHPUnit, this is fine. In Drupal case, we don't need to put the script file in there, but the autoloader and light bootstrapper for tests may live in here.

So we could actually have:

core/lib/Drupal/ # Drupal namespace
core/tests/DrupalTest/ # DrupalTest namespace, did put Test suffix to be PSR-0
scripts/runtests.sh

Having the tests not being self-contained is not an issue for core, core is a unified packaged. For modules, using PSR-0 with the same folder structure on a per module dir basis seems good enough to me, if we ever agree about modules PSR-0 structure.

Crell’s picture

So to clarify sun's point in #3, we can certainly have test classes live in the Drupal\Tracker\Test[s] namespace but live on disk in .../modules/tracker/tests rather than in lib. That works just fine, and would actually be my preference.

Berdir’s picture

So... I'm pretty sure we should either split the following discussion out into a new issue called "Convert Testing module to PSR-0" or rename this issue and simply use TrackerTest as the example implementation that is converted with that.

1. The provider thingy is in, but the discussion points towards having to register second Namespace per module, which could be done in simpletest_init() I guess for each enabled module. Not sure about a fast way to do that but it would be kinda nice in the way that test classes are only registered in the system when simpletest.module is enabled, as opposed to right now. Or we do it only for modules which provide tests, see below.

2. Hm... Right now, Simpletest extracts it's classes from the registry table. Obviously, there is no way to do anything like that when using PSR-0. As fancy as the current system is, it has a serious drawback: The test overview page needs to load every single test class in the System and call the getInfo() method of it. This is not only slow, it also means that if you have a missing or broken test class, the test overview page dies with a fatal error.

So, why not go back to the roots and add a simple hook_simplenews_info() (we already have hook_simpletest_alter, which we might want to rename to hook_simpletest_info_alter(), anyway). Then, the static getInfo() methods are moved into that hook implementation, keyed by the test class name. That could be fully qualified, or we could maybe even hardcode the Drupal\$module\Tests namespace as a default somehow (maybe by having a 'namespace' array property that we can add a default value for). Might lead to weird conflicts/overrides, though.

Thoughts?

Berdir’s picture

Status: Needs work » Needs review
FileSize
2.98 KB

Proof of concept implementation that seems to work, I can execute the Tracker tests through the UI.

- Added the (yet undocumented) hook_simpletest_info()
- Implemented in tracker.module
- simpletest_init() defines tests namespace for each module that implements said hook. Might even be possible to call this on-demand when required, not sure.
- The getInfo() info is currently duplicated, because the method is still called directly on the test report page.

I guess it makes sense to split simpletest_test_get_all() into two different functions. A basic one that returns tests keyed by the class name and works similar to e.g. entity_get_info() (called without argument returns all, passing a class name in returns the information just for that one). Then we can call that directly on the reports page and drop static function getInfo() completely. And then a second function on top of that that groups the results to be used for the tests overview page.

Crell’s picture

I am skeptical about hook_simpletest_info(). Wouldn't that make moving to a PHPUnit base (as has been suggested and I agree we should do) even harder?

Berdir’s picture

If you have better ideas, I'm all ears :)

There needs to be *some* way to detect test classes. The only other option I can think of is going back to what we did in 6.x, which is parsing files, e.g. all files within $module/tests and check if they contain a class with a getInfo() method.

effulgentsia’s picture

How about file_scan_directory() for all files within $moduledir/tests/Drupal/$modulename/Test/? No need to parse these files: they're PSR-0, so 1 class per file, and we know the class name from the file name.

Also, this needs to be done for all modules, not merely the enabled ones. You could have tracker.module disabled when navigating to admin/config/development/testing, but the tests should still be listed there.

Crell’s picture

#10 makes a lot of sense to me. Simple and effective. Just remember to make it a recursive scan from that root, since test classes could be in subclasses.

Also, there does need to be some way to note a particular class as not a test case but simply a support class for a test case. We have one or two of those lying around somewhere.

Berdir’s picture

Assigned: Unassigned » Berdir

Ok, will give that a try :)

That check already exists and is done by checking if the class in question has a static testInfo() method. Which means that we do not need *parse* but still include the class in question, which means that a php error in any of these will kill the test overview page and it will be slow but yes, we do need tests of disabled modules as well.

Berdir’s picture

FileSize
5.99 KB

Ok, here is a patch that seems to work for me. Not exactly the nicest code that I've ever written, but works for a start.

I tried to two things to keep the impact on the system due to tests as low as possible (after the initial test class loading):
1. Maintain a simpletest_modules cache that contains a list of modules which provide tests. To prevent having to add test namespace for dozens of modules (once you add contrib to the game) that don't have any.
2. Only register them on-demand, when either calling simpletest_test_get_all() (= test overview page), actually running tests and viewing results.

We can easily change 2. and register them in hook_init() if that turns out to be too fragile. It's not like you'd want to have the testing module installed on a productive site anyway. Same for 1., although that would mean to query the system table directly for every request as there is no API to fetch all available modules other than actually parsing the filesystem, which would be even worse.

Feedback?

Berdir’s picture

The first patch actually showed the problem with the hook nicely, as the Tracker tests weren't found/executed.

Let's see if this is any better...

Status: Needs review » Needs work

The last submitted patch, tests_psr0_2.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
6.79 KB

Ok, the run-tests.sh script needs to be fixed as well to a) register the namespaces and b) quote the class name to keep \'s.

Works for me locally.

effulgentsia’s picture

Status: Needs review » Needs work
+++ b/core/modules/simpletest/simpletest.module
@@ -356,6 +394,25 @@ function simpletest_test_get_all() {
+    if ($cache = cache()->get('simpletest_modules')) {
+      $loader = drupal_classloader();
+      foreach ($cache->data as $module) {
+        $loader->registerNamespace('Drupal\\' . $module . '\\Tests', DRUPAL_ROOT . '/' . drupal_get_path('module', $module) . '/tests');
+      }
+    }

It seems a bit fragile to do nothing if the cache()->get() doesn't return anything. We're relying that simpletest_test_get_all() ran previously, and the cache hasn't been emptied since then. We're also relying that there's a working cache backend.

Would the test performance be too degraded if we registerNamespace() every module, even ones without PSR-0 tests? That could remove the need for the 'simpletest_modules' cache entirely. Otherwise, maybe there's a way to bring the file scanning from simpletest_test_get_all() and cache checking into the same function?

+++ b/core/modules/tracker/tests/Drupal/tracker/Tests/TrackerTest.php
@@ -2,9 +2,13 @@
+namespace Drupal\tracker\Tests;

I think our naming standard is to use singular: Drupal\tracker\Test.

effulgentsia’s picture

Also, the result page on http://qa.drupal.org/pifr/test/253034 shows:

Drupal system listing (CommonDrupalSystemListingTestCase) [Common]	6	0	0
Drupal\tracker\Tests\TrackerTest	234	0	0
drupal_add_feed() tests (CommonDrupalAddFeedTestCase) [Common]	6	0	0

So, not picking up the class's getInfo(), possibly related to the cache issue in #17?

Berdir’s picture

Status: Needs work » Needs review
FileSize
33.43 KB

Changed namespace to "Test", what about the "tests" directory? Singular does sound strange there..

Note that many modules currently already have a tests folder with test modules and other related things in there. We do have the lib folder to not have a mix of PSR-0 and other code in the same directory, that's currently not the case for tests. Renaming the folder to test would fix that but then we'd have modules with test/ and tests/ which is kinda confusing.

The cleanest solution would probably be to move everything other than test classes out of the tests/ folder into something else, not sure what exactly, though.

#18 I'm not sure if that's related. If the namespace wouldn't be registered, it would not be possible to run the tests in the first place. And it's displayed correctly for me when calling run-tests.sh, also after a cache clear. I think that is test-specific code that needs to be updated to conditionally call the that function when it's available. For testing, let's see if calling that function in simpletest_init() helps. I also changed the register namespace function to call simpletest_test_get_all() in case of a cache miss. That should be loop-safe because even if there are no test modules, it will save an empty array to the cache.

Another thing for later: we could register core/tests as Drupal\Test and move most of the tests in simplenews/system over there. Later, of course.

Berdir’s picture

Note: Calling simpletest_test_get_all() in hook_init() essentially means that if you have a test class with a PHP error, it will not only kill the test overview page but everything. I really think we should try to avoid that. Calling simpletest_register_test_namespaces() without the cache miss fallback would be ok and in my testing, I wasn't able to get an error because neither through the UI nor run-tests.sh is it currently possible to execute a test without a call to simpletest_test_get_all() first.

effulgentsia’s picture

Status: Needs review » Needs work

Thank you for persevering with this.

- #19 includes changes to number.module and other stuff you were likely working on from other issues :)
- http://qa.drupal.org/pifr/test/253169 still shows a summary line without getInfo() info.
- If above can be fixed without a simpletest_init(), I agree that would be preferred.
- I think "tests" (plural) as a folder name is fine, or at any rate, changing it is out of scope for this issue: it's only namespace/class naming where we're standardizing on singular. #1436384: Rename folders under /core to be consistently singular is the issue for changing non-PSR-0 folder singular/plural naming.

Berdir’s picture

Ah, forgot to rebase my feature branch and the -M switch, sorry about that. Was getting too late yesterday ;)

- Will need to talk with jthorson/rfay about the pifr reports
- ok, tests is fine with me, what about non-PSR-0 code in that folder like test modules?

sun’s picture

Status: Needs work » Needs review

Patch in #19 has unrelated changes unintentionally merged in.

  1. The hook_init() approach is not appropriate and doesn't fly at all for me. Just because SimpleTest module is enabled does not mean that I plan or want to run any tests on every single request.
  2. Instead, we want to introduce a simpletest_classloader(), which we register next to the default drupal_classloader().

    That is, because there seems to be some desire to put test classes outside of the regular PSR-0 structure, so the regular classloader does not kick in at all.

  3. We not only need to register all namespaces of (enabled and disabled) modules in all possible locations, we also need to register their regular runtime PSR-0 namespaces.

    That is, because test classes may want or need to instantiate classes that are shipped with the module being tested. This also means that simpletest's additional classloader needs to partially duplicate the existing registry of drupal_classloader().

  4. I'd recommend to forget about caches and premature performance optimization here for the time being. A reliable and working implementation needs to come first. For executing a test, we only need a working simpletest_classloader() implementation that's able to lookup and load particular/known test classes. Implementation-wise, that's basically the same as the runtime drupal_classloader(), but either querying the {system} table for all known modules, or invoking system_rebuild_module_data(), and registering the namespaces for everything.

    Obviously, system_rebuild_module_data() is slower and actually performs more actions than we want. However, it is reliable, always available, and does not rely on any dependency (not even Drupal being installed) to work. Lacking a better alternative, we might want to go with calling the private _system_rebuild_module_data() sub-function that actually performs the lookup (without touching/rebuilding the actual system data).

  5. The additional simpletest_classloader() should only be registered when it is needed. Which most likely means in the batch operation callback only.
  6. For discovery and the listing of test classes, the proposed filesystem scan sounds fine to me. However, we definitely need to load all the class files that are found, in order to check whether a particular test class/file is a pseudo-abstract base test case class for other test cases, which is identified by whether there is a static getInfo() method or not.

    The simpletest_classloader() needs to know about these classes, whereas the test case listing needs to skip/omit them.

    FWIW, I'd like to replace the special static getInfo() method with phpDoc/annotations in #697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit) - details to be discussed over there.

  7. The current ./tests directories not only contain arbitrary helper files for individual tests (such as dummy asset files being uploaded and actually whatever is necessary for any particular functional test), they also contain entire testing modules (which, in turn, may contain tests again).

    The desire to put test classes into ./tests, along with #1299424: Allow one module per directory and move system tests to core/modules/system, directly conflicts with the "policy" we agreed on during the PSR-0 namespace discussions; namely, to not mix the namespaced classes code with arbitrary other files in the filesystem structure.

    FWIW, I don't see the point in the desire to put test classes somewhere else and believe it's, again, a naive idea of blindly following what others do (which, btw, is the mindset that led to the holocaust and 2nd world war). Not doing so would make our lives a lot easier, since we don't have to reinvent the wheel and duplicate the already existing class loader and entire file structure in order to load files that would almost be readily available otherwise. It would also resolve the issue of mixing namespaced code with arbitrary test support files. But I've given up on discussions like this, since any kind of logical sanity is overruled either way.

Berdir’s picture

@sun The hook_init() approach was just a test to see if that fixes the reported issues with the testbot, which it doesn't. The previous patch registers those test namespaces only on demand, when actually running tests or otherwise requiring the classes. In fact, that code is still there.

The current approach would work fine when instead of using the tests/ directory, placing them inside lib and simply enforce the $module\Test namespace. Then we would simply have to add the default module namespace for disabled modules as well.

Happy to discuss this in more detail today evening in IRC.

Crell’s picture

sun, you're posting in comment #23. That's way too early to get to Godwin's Law.

That said, a clear advantage of a separate class path for tests is that we can register only enabled modules for their code, but all available modules for simpletest. Since we need to enable a module anyway as part of the test, we should be fine with having the test class loader *just* look at the /test directory and ignore /lib, so that we can run tests on all modules but only have access to main classes from an enabled module, which is the current behavior already.

Berdir’s picture

FileSize
6.48 KB

1. As I said, that was just a try to see if that fixes the issues with the pifr report. Doesn't help, so removed again.

2. That's what simpletest_register_test_namespaces() currently is doing basically. It's not on the same level as drupal_classloader() and I don't see why it should be.

3. That is true. Given that, I think it's actually much simpler to go with test classes in lib/Drupal/$module/Test. This means we can simply the function mentioned above and simply call drupal_classloader_register() for disabled modules, as it's done for enabled ones already. This is similar to the current registry_info_alter() behavior, which exposes files[]= definitions of disabled modules.

4. Ok, removed *all* performance optimizations and reduced to the bare minimum. Given that Simpletest is a drupal module, I don't see why we shouldn't be relying on Drupal though, so I've queried the system table. IMHO, we should at least re-add the repeated call protection to not re-add the namespaces multiple times, e.g. when calling multiple batch operations on the same request. That's possible for simple DrupalUnitTest test cases I guess.

5. No, that's not enough, we also need it when search for tests on the test overview page, on the test result page and in run-tests.sh, which also requires adding "" around the test name to prevent from losing the \. All of which this patch is already doing.

6. Which is exactly what the code is already doing, we don't even need to touch that, just add the found PSR-0 test classes to the registry classes array.

7. As mentioned, this patch uses the lib directory, since we could possibly need it anyway if tests somehow relate to the actual classes/interfaces (like a test-only implementation of an interface kept in lib/). Makes the register namespace function a bit simpler but the differences are really minor. I kinda do prefer this, as this means we can keep the test modules and stuff in a tests/ directory.

I hope this patch helps to prevent the third world war ;) Certainly can't get much simpler than this...

Berdir’s picture

ToDo: Open an issue in http://drupal.org/project/issues/testbot wich cross-reference to this one and try to get the pift review page to work properly.

catch’s picture

+ simpletest_register_disabled_modules();
Modules don't have to be disabled or enabled to be tested, they can also be not installed.

Berdir’s picture

That's correct, the name is wrong. The function does work correctly, though :)

Berdir’s picture

sun’s picture

Status: Needs review » Needs work
+++ b/core/modules/simpletest/simpletest.module
@@ -159,6 +159,7 @@ function simpletest_run_tests($test_list, $reporter = 'drupal') {
 function _simpletest_batch_operation($test_list_init, $test_id, &$context) {
+  simpletest_register_disabled_modules();

@@ -310,6 +314,10 @@ function simpletest_log_read($test_id, $prefix, $test_class, $during_test = FALS
 function simpletest_test_get_all() {
...
+  simpletest_register_disabled_modules();

+++ b/core/modules/simpletest/simpletest.pages.inc
@@ -183,6 +183,7 @@ function theme_simpletest_test_table($variables) {
 function simpletest_test_form_submit($form, &$form_state) {
...
+  simpletest_register_disabled_modules();

@@ -233,6 +234,8 @@ function simpletest_result_form($form, &$form_state, $test_id) {
+  simpletest_register_disabled_modules();

+++ b/core/scripts/run-tests.sh
@@ -363,6 +363,8 @@ function simpletest_script_run_one_test($test_id, $test_class) {
+    simpletest_register_disabled_modules();

The amount and placement of calls to simpletest_register_disabled_modules() concerns me a bit... are we sure that all of them are required and/or that these are the best locations to put them?

At least the one in _test_get_all() should be moved inside the !$groups condition, so the registration happens only once.

+++ b/core/modules/simpletest/simpletest.module
@@ -320,6 +328,31 @@ function simpletest_test_get_all() {
+      // @todo: can profiles/themes have tests as well?
+      $module_list = db_query("SELECT name, filename FROM {system} WHERE type = 'module'")->fetchAllKeyed();

As of now, profiles can have tests, too. However, install profiles are registered as type 'module' currently, so this query happens to not break anything.

However, I'd recommend to just simply drop the type condition, since that automatically gains us test support for extensions of all types (including themes).

+++ b/core/scripts/run-tests.sh
@@ -396,7 +398,7 @@ function simpletest_script_command($test_id, $test_class) {
-  $command .= " --php " . escapeshellarg($php) . " --test-id $test_id --execute-test $test_class";
+  $command .= " --php " . escapeshellarg($php) . " --test-id $test_id --execute-test \"$test_class\"";

We should use escapeshellarg() for $test_class here now.

Berdir’s picture

Yep, the one in test_get_all() can be moved. Right now, every single of them is needed, because we are using the test classes in different ways, the test_get_all() method is only used in the overview and init part of run-tests.php, the review form calls testInfo() on the class directly and not the cached information from test_get_all(), submit needs it too and run-tests obviously as well.

I'm fine with removing the type condition.

Suggestions on a better/correct name for simpletest_register_disabled_modules() anyone?

effulgentsia’s picture

Suggestions on a better/correct name for simpletest_register_disabled_modules() anyone?

How about simpletest_classloader_register()?

sun’s picture

rename from core/modules/tracker/tracker.test
rename to core/modules/tracker/lib/Drupal/tracker/Test/TrackerTest.php

I'd like to see the "Test[Case]" suffix removed from the file and class name. Additionally, I want to see the module name prefix removed from the file and class name.

The Tracker tests are a poor example, so not worth to discuss based on them. But other test cases are sufficiently structured to make sense of my envisioned naming - for example:

CommentThreadingTestCase would currently become Drupal\comment\Test\CommentThreadingTestCase, which contains plenty of pointless repetition.

Instead, I want to see Drupal\comment\Test\Threading in a dedicated Threading.php file.
One test case class per file, so one file denotes one test case.

webchick’s picture

Hm. Not sure about that. Having "Test" in the class name clearly denotes what the class is about. A "Threading" class in an IDE would give the wrong impression. Agree that "Case" is not required, though.

effulgentsia’s picture

In #1507828: [policy, no patch] Revised standards for class naming within namespaces, we settled on a policy that calls for:

Classes and interfaces should have names that stand alone to tell what they do without having to refer to the namespace, read well, and are as short as possible without losing functionality information or leading to ambiguity

Neither the issue nor the policy mentions test classes though. So here seems like a fine place to decide on that.

Crell’s picture

Per the discussion in #1507828: [policy, no patch] Revised standards for class naming within namespaces a "*Text" suffix on the class short name is probably reasonable. Agreed about leaving out the module name and "Case", though. (Unless there's only the one test class, in which case ModulenameTest seems like a reasonable convention.)

RobLoach’s picture

Let's look at a system that has already solved this problem. Symfony/Component/Console/Tests has [ClassName]Test to test everything about [ClassName], keeping the same namespaces prefixed by "Tests". That is PHPUnit, but we can certainly take inspiration from it.

Taking that into consideration, it seems like CommentThreadingTestCase might translate to Drupal/comment/Tests/ThreadingTest.php.

Berdir’s picture

Yes, I think the Test suffix makes sense. e.g. SelectQueryTest for the SelectQuery class. Without that, they would have the same name so you would have to alias them in the test, which is completely silly :)

Not sure about the case where there is only one test class for a module, e.g in here. Adding another test class would mean that it's not the only one anymore so you'd have to rename it. I don't have a better suggestion, though.

Another thing is Tests vs. Test in the namespace. Initially it was Tests, then I changed it to Test because we're currently using the singular for other namespaces (e.g. Component, not Components). Happy to re-roll to whatever we decide to pursue. IMHO, we can still improve and refactor how test classes are detected and registered in SimpleTest. However, deciding to switch from Tests to Test after 50% of the core tests have been converted would be... not much fun. So we should try to agree on something now and then stick to it :)

jhodgdon’s picture

RE #38 - CommentThreadingTestCase should probably be Drupal/comment/Tests/CommentThreadingTest.php, right? If it's really going to stand alone, it should have Comment in the name, since "threading" could apply to other things (and it's also a standard computer science term having to do with multi-threaded processes).

effulgentsia’s picture

http://drupal.org/node/608152 doesn't have examples for modules, but I'm concerned that if we require full disambiguity across modules, we'll end up with the module name being added to every class. Perhaps we should only expect disambiguity within a module, in which case I think ThreadingTest would be fine unless comment.module also had tests for other kinds of threading.

jhodgdon’s picture

I guess for tests it isn't that essential, but in general the idea was that class names should stand alone. Period. So that if you do in some module:

  $x = new LocalFileTransfer();

it's a lot clearer than for instance

  $x = new Local();

which is one of the actual examples that triggered the whole discussion on that standards issue.

Tests probably don't need to stand alone as much, though because it's not like you're going to be using them in other code. So maybe we need to add an exception to the standards on http://drupal.org/node/608152 for the names of test classes, to say that they only need to stand alone within the context of the module they're testing, and that they should definitely end in "Test"? Then we could have ThreadingTest in the context of the comment module.

So, here's a proposal of what to add to http://drupal.org/node/608152 in the naming section at the top:
---
Item to add to the main list, just after the one that says interface names end in Interface:

* Test class names should end in Test.

Line to add to the bullet points on the current item #8 that talks about unambiguous names:

* Exception for test classes: Test classes only need to be unambiguous within the context of the module they are testing.

Example to add to #8's table of good/bad names:

Namespace: Drupal/comment/Tests/
Good: ThreadingTest
Bad: CommentThreadingTest (only needs to be unambiguous in comment context), CommentThreading (needs to end in Test)
-------------

Thoughts?

webchick’s picture

#42 wfm!

effulgentsia’s picture

I like #42. How about when a module only has one test class? Should it be just named Test or [UppercaseModuleName]Test or [FindAWordToDescribeItThatsNotTheModuleName]Test? We need that answered for TrackerTest.

Also, some modules have multiple tests, one of which is currently just named with the module name. For example, we currently have MenuTestCase and MenuNodeTestCase. What do we rename those to?

RobLoach’s picture

MenuTestCase: Drupal\menu\Tests\MenuTest
MenuNodeTestCase: Drupal\menu\Tests\MenuNodeTest

The class name doesn't really matter, as long as it's in the Tests namespace, and is suffixed with "Test". TrackerTest should really not be a problem since there's currently only one test class for it (part of the reason why I choose it).

webchick’s picture

Right. I think it's just [FindAWordToDescribeIt]Test, which in the case of a single test for an entire module's capabilities in TrackerTest. In other cases, will be something different. Let's please not get prescriptive over every detail, and cause more reasons for things to get marked CNW. :)

jhodgdon’s picture

+1 on #46. To expand, I think #42 will not cause too much bikeshedding in the case of naming TrackerTest. I also think the examples in #45 are fine, and don't violate #42 either (I think -- I'm assuming MenuTest covers most menu testing, and MenuNodeTest is some menu stuff specific to nodes, so if that is what they are, then I think we've covered the "this describes unambiguously" standard). :)

effulgentsia’s picture

Ok, then as per #39, I think the last naming thing to decide here is Drupal\tracker\Test\TrackerTest or Drupal\tracker\Tests\TrackerTest? The former makes more sense to me from the standpoint of keeping all namespace and class names singular, and not making people have to think about why Tests is an exception to that rule. However, the latter is how Symfony does it, and I don't know if there's any compelling argument for being consistent with Symfony.

Berdir’s picture

Status: Needs work » Needs review
FileSize
5.6 KB
6.43 KB

From what I see, Tests seems to receive more support (It's also explicitly mentioned in #42, in case you didn't notice ;)).

So here is a re-roll that contains:

- Renamed to simplenews_classloader_register() and moved into !$groups block
- Used escapeshellargs()
- Back to Tests namespace
- Removed the module condition from the system queries, removed the @todo and added one to remove the registry query once ALL the things are PSR-0.

Interdiff attached, which is almost as big as the actual patch ;)

jhodgdon’s picture

For the record, I am not officially endorsing Test vs. Tests -- I am ambivalent, and just used Tests in the #42 proposal because that is what was in the previous examples others had posted. :)

Anyway, I took a quick look at the patch, and noticed this:

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerTest.php
@@ -2,9 +2,13 @@
 
 /**
  * @file
- * Tests for tracker.module.
+ * Definition of Drupal\tracker\Tests\TrackerTests

That last line should end in .

sun’s picture

Status: Needs review » Needs work
+++ b/core/modules/simpletest/simpletest.module
@@ -391,9 +391,9 @@ function simpletest_test_get_all() {
-function simpletest_register_disabled_modules() {
+function  simpletest_classloader_register() {

Double space.

Regarding Test vs. Tests, I really don't want to bikeshed. However, as we're going to convert all tests throughout Drupal core and likely also contrib after this issue, we should be sure on that. Like @effulgentsia in #48, I already mentioned in #3 that the plural part in the namespace is an inconsistent and confusing exception. When comparing two namespaces, this becomes apparent:

Drupal\Component\Graph
Drupal\comment\Tests\ThreadingTest

If it was about a ./tests directory next to ./lib, then "tests" would work fine. But since we have a singular naming standard for PSR-0 namespace components, we should not break that standard, unless strictly necessary. Our track record of inconsistency big enough already.

---
Off-topic: While thinking through this, I wondered how tests for core components would work with this pattern, and it looks like it works fine, merely using a wildcard search pattern on the third namespace component:

Drupal\Component\Graph
Drupal\Component\Graph\Test\GraphTest
...
Drupal\Component\*\Test\*
Drupal\Core\*\Test\*

That would allow us to move most of the test case classes in core/modules/system/tests (formerly simpletest/tests) into the appropriate locations.

webchick’s picture

I'd be more comfortable renaming "Component" to "Components" than "Tests" to "Test" personally. There's already a precedence for this in Symfony.

webchick’s picture

Ha, actually Symfony does: Symfony/Component/Console/Tests/Tester

Let's do the same: "Component" and "Tests"

RobLoach’s picture

This is how Symfony's set up would translate to Drupal...

core/lib/Drupal/Core/Utility/CacheArray
core/tests/Drupal/Tests/Core/Utility/CacheArrayTest
core/lib/Drupal/Component/Graph/Graph
core/tests/Drupal/Tests/Component/Graph/GraphTest
modules/search/lib/Drupal/search/SearchQuery
modules/search/tests/Drupal/search/Tests/SearchQueryTest

PSR-0 namespace registration:

  • core/lib is registered as Drupal
  • core/tests is registered as Drupal/Tests
  • %modulename/lib is registered as Drupal/%modulename
  • %modulename/tests is registered as Drupal/%modulename/Tests

I'd be more comfortable renaming "Component" to "Components" than "Tests" to "Test" personally. There's already a precedence for this in Symfony.

No. Symfony uses "Component", and "Tests". Let's stick with what Symfony uses. [Edit] Ah! Cross post :-) . Okay, carry on lol.

effulgentsia’s picture

Re #54: according to https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Htt..., Symfony does Symfony\Component\$ComponentName\Tests\$TestName, not Symfony\Tests\Component\$ComponentName\$TestName. That doesn't affect this issue, but just pointing out so we're on the same page when converting non-module tests.

Also, according to that github link, the code and tests are all in /src. Where do you see it getting split into /lib and /tests?

Finally, in #48, I asked what's the compelling reason for us to follow Symfony's choice to pluralize Tests when they don't pluralize any other namespace name. Is it because we plan on adding Symfony tests to Drupal's codebase and we therefore want consistency across files in our repository? Is it to make things a little easier for Symfony developers who want to write tests for Drupal? Is it because we think it's logical to have Tests be plural when nothing else is? Is it to allow singular Test to refer to actual code (e.g., during normal Drupal operation, a class that tests for an active network connection or whatever)? Is "Symfony does it that way" enough of a reason for us to introduce inconsistency into our own codebase? I'm not opposed to any of these reasons. I just want to make sure we have a reason that we feel outweighs the potential confusion Drupal contributors will face when seeing an exception to the pattern of singular namespace names.

sun’s picture

Is it to allow singular Test to refer to actual code (e.g., during normal Drupal operation, a class that tests for an active network connection or whatever)?

That's finally a technical argument I can get behind. Based on that, the inconsistency of Tests makes sense and can be explained to developers.

@Rob Loach: I already clarified in #3 that Symfony's structure is custom, most likely caused by a lack of will to care for DI for tests, and therefore should not serve as an example. But anyway, let's discuss the structure for core component tests in a separate issue.

This patch looks RTBC to me after fixing the double space.

jhodgdon’s picture

The patch also needs fix in #50.

Meanwhile, it looks like everyone is in agreement now on the coding standards update in #42, so I went ahead and added it to the Naming section at the top of http://drupal.org/node/608152

Berdir’s picture

Status: Needs work » Needs review
FileSize
6.43 KB

Ok, fixed the two issues.

RobLoach’s picture

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerTest.phpundefined
@@ -2,9 +2,13 @@
  * @file
- * Tests for tracker.module.
+ * Definition of Drupal\tracker\Tests\TrackerTests.
  */

TrackerTest :-)

-18 days to next Drupal core point release.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

catch’s picture

Title: Convert Tracker tests to PSR-0 » Change notification for: Convert Tracker tests to PSR-0 / PSR-0 test class discovery
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

It looks like from #1477218: Convert Tracker tests to PSR-0 / PSR-0 test class discovery that we're just missing the group in the report, if this is the case, then I agree that shouldn't hold the patch up. If we find some other side-effect we might have to roll this back though, but committed/pushed this for now.

This will need a note to explain where the test classes should go now - it's a bit different from the normal PSR-0 one since we've got the extra discovery stuff in SimpleTest, but not sure if it should be a separate change notification or added to the existing one.

xjm’s picture

I'd say create a separate change notification referencing the other one.

we're just missing the group in the report

This scared me, but I checked and it has on a green line:
Drupal\tracker\Tests\TrackerTest

I'd like to see what happens when we have a failed assertion in that file. I'll create a dummy issue to test that.
Edit: #1543250: Test issue for testing PSR-0 test classes with QA

xjm’s picture

See http://qa.drupal.org/pifr/test/259819. That seems fine to me.

Berdir’s picture

Status: Active » Needs review

Started a change record, please review: http://drupal.org/node/1543796.

Will need to be updated once Simpletest has been converted and again when we have a agreed on a way to define classes for Drupal\Core and Drupal\Component stuff. By the way, is there a way for marking change records as obsolete? For example, http://drupal.org/node/1540510 will be pointless once we have moved them again to core/lib (hopefully).

sun’s picture

Title: Change notification for: Convert Tracker tests to PSR-0 / PSR-0 test class discovery » Convert Tracker tests to PSR-0 / PSR-0 test class discovery
Priority: Critical » Normal
Status: Needs review » Fixed

Excellent, thanks.

Status: Fixed » Closed (fixed)

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

sun’s picture