Allow PSR-0 test classes to be used in D7 (followups)

This makes core trying to use contrib module code and proceed to a runtime check, this taints core. Maybe we could backport the classloader (both drupal_classloader() and the Symfony component) into Drupal 7 first.

Backporting the autoloader without the vendor directory for contrib sounds like something doable. This code still taints the core, it shouldn't be commited as it is.

Leaving an hardcoded reference to a contrib project into core is a no go. The module can (and probably will) evolve (thus breaking API) until at least D8 is stable itself. Even thought you are the module maintainer, if it wasn't you, you probably wouldn't allow it. Core will be tainted.

drupal.test-psr0.0.patch queued for re-testing.

Do we have any actual use cases for this for Drupal 7? Something that it actually tests?

#1674290: PSR-0 tests in contrib modules are not found is not enough to find and support PSR-0 tests in D7. For this we need the simpletest_get_all() hunk from #0.
The d.o testbots (run-tests.sh with the --file option) depend on this function when determining validity of the discovered tests. So how about focusing on this hunk first?

From simpletest_script_get_test_list():

      // Check for valid class names.
      foreach ($all_tests as $class_name) {
        $refclass = new ReflectionClass($class_name);
        $file = $refclass->getFileName();
        if (isset($files[$file])) {
          $test_list[] = $class_name;
        }
      }

+++ b/modules/simpletest/drupal_web_test_case.php
@@ -443,7 +443,8 @@ abstract class DrupalTestCase {
+      $class = str_replace('\\', '_', get_class($this));

@@ -462,7 +463,8 @@ abstract class DrupalTestCase {
+    $class = get_class($this);

I realize this is bikeshed-territory, but it feels strange, to have the first named $class even though it's not the valid class.

Other than that, this looks great.

If people are against adding this explicitly, we could also add a hook_simpletest_classes_alter() or whatever so that this could be added from contrib. Since this is not some arbitrary contrib module but an established standard (even though D7 doesn't use it), I personally don't see any reason not to include this directly in core, though.

Note:
xautoload (7.x-2.x-dev) does this already, since today.
And yes, it also finds tests in modules that are not enabled.
No core patch involved.

See #1751540: Expose test classes to Simpletest using hook_simpletest_alter().

--------

Some comments about the patch above:
We register the full namespace \Drupal\(module), instead of just \Drupal\(module)\Tests. Is this intended?
(xautoload currently only works for \Drupal\(module)\Tests on disabled modules, so I probably need to change that.)

I guess, the preferred logic would be
- register \Drupal\(module) for enabled modules.
- register \Drupal\(module) for disabled modules that have a lib/Drupal/(module)/Tests folder.
- register nothing for other disabled modules.

This module seems nice, I'll take a look at it later, thanks!

xautoload (7.x-2.x-dev) does this already, since today.
And yes, it also finds tests in modules that are not enabled.
No core patch involved.

Does it load tests when tests are invoked by run-tests.sh, as done by the d.o. testbot?

#16
I'm not sure what needs to be done there.
If you can describe this feature in more detail, I will work on it :)
(if it is not already the case)

@Eric_A (#16):
Update: The answer is: No, it does not work in the current version of xautoload.
This has several reasons..

@sun (#18):
I actually did make it work, "sort of", just now, locally.
Zero modifications to core, but a number of modifications to xautoload, that I was planning to do anyway.
Except for the problem with the backslashes. I had to work around this with a really ugly hack, and with that included, I can not seriously claim that xautoload alone solves the problem.

--------------

Some observations regarding your post #18:

As the issue summary states already, various fixes had to be applied to D8, just to make Simpletest compatible with the backslashes being contained in namespaced class names.

Indeed I wasn't aware of that.
Strange though, it seems that only the run-tests.sh is affected, not the usual web-based simpletest. I never had problems with that.
Strange also that the patch above does not change anything in run-tests.sh, if that is the point where it fails.

Besides my little experiment, I still support changing this code in core, so that classes with backslash are not scrambled.
This could be a first step, and then we move on from there.

hook_simpletest_alter() does not help at all, since hooks are only invoked in enabled modules, and run-tests.sh does not enable any extra modules in the parent-site/test-runner, and you have no way to influence that.

Then it is strange that it did work for me.
In my tests, I put a number of debug statements, and it showed that all modules were still enabled.
At first I thought maybe it is only bootstrap modules, but no, it was all of them.
This is not really surprising, since run-tests.sh uses drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL) all over the place.

What I did: On a D7 site with xautoload enabled, I went to the root folder and started php scripts/run-tests.sh "X Autoload" (to run the stupid arithmetics test that is included by default) and then php scripts/run-tests.sh "X Autoload Example" (a module which is disabled on this site, and contributes another stupid example test).

It could be that what the test bot does is yet something else, so that my experiment misses the point.
But in #18 you were talking only about run-tests.sh, and this is what I tested.

Patch.
In my limited experiments, this is about the only change in core that contrib needs, to solve the problem without ugly hacks (*).

I looked at D8, they basically do the same, but with a lot more changes around that. We could still get more D8 code in as a follow-up.

The patch seems quite safe, and I would suggest to commit that as an intermediate step.
Even if that is not sufficient, it is also not wrong.

-------------

(*) "ugly hacks":
If anyone is interested, the hack I mentioned in #19 was based on preg_match('/^Drupal(.+)Tests(.+)$/', ..) in the class loader. That's obviously fragile.

Ok, the green is not really surprising, this patch does not change much..

I did some more work on xautoload, and confirm that scripts/run-tests.sh does appear to work with the latest 7.x-2.x, combined with the patch in #20. The unit test and web test in the disabled xautoload_example are detected and run (with all passes, as to be expected, these are trivial tests). I could add more fancy stuff into the web test, but basically I don't see why that should break.

@sun.
And again, noone in this issue ever mentioned PIFR.

If you don't want to enable a contrib module, then contrib cannot help you, obviously.
I don't see how classloader would be any different from that.
So, either change PIFR to enable xautoload (or classloader, if you prefer that), or backport the complete D8 PSR-0 to D7 core.

EDIT:
I shall refrain from posting until I have read more about the testbot.

@Rob Loach #10, I have also a use case in a contrib module, check the 7.x-3.x branch of WoW API module.

Thanks for the classloader module sun and a huge + for this patch. Forward Compatibility++++

Just a question for my personnal curiosity, what are the differences between the 35ko module and this snippet of code:

<?php
// Registers PSR-0 style classes.
spl_autoload_register(function ($class) {
  $class_fragments = explode('\\', $class);

  if ($class_fragments[0] == 'Drupal') {
    include drupal_get_path('module', $class_fragments[1]) . "/lib/$class.php";
  }
});
?>

I mean, functional differences.

Edit: I opened the classloader module to see myself :)
The differences are for the sake of answering, the use of APC class loader when in prod, the possibility of loading far more than just module defined lib/ folder, but also vendor libraries, etc. It also embed the full ClassLoader component that's why the size is high.

Re #22 and #23
http://drupalcode.org/project/project_issue_file_review.git/tree/refs/he...
This thing looks big, and I don't know where to start studying.

Is there any possibility to modify PIFR, and let it enable a contrib module that can find the PSR-0 tests?
There are different ways imaginable - e.g. it could do one test run with this module enabled, and another test run with this module disabled - if we are afraid of side effects.

Another idea.
What about a simple PSR-0 class loading solution in D7/D6 simpletest module?
This minimal class loader would only load test classes, not anything else.

Benefits:
- we avoid dependency on contrib modules.
- we don't harm the testability of class loading solutions in contrib. (*)
- it would be the same mechanic for local test runs and for testbot.
- any modifications we do here will only affect people who enable simpletest.

(*) E.g. if a module misses to declare a dependency to xautoload or classloader, but one of these modules is already enabled due to the test bot, the test will pass, even though the module is broken.

That's exactly what the patch in the original post does. :-)

Hm, not really.
The original patch uses drupal_classloader_register(), which it expects to get from classloader module.

What I propose is to implement a simple standalone class loader only for tests, not depending on any contrib module.
Normally I would disagree with this for (really minor) performance reasons. But in case of tests, who cares about a few milliseconds extra.

I might produce a patch ..

Patch: A minimal class loading solution only for PSR-0 tests classes.

The idea is that most other things can be done in contrib:
- PIFR already does discover PSR-0 test classes in D6 and D7, it just can't autoload them.
- Local test runs can use hook_simpletest_alter() to discover PSR-0 test classes (like how xautoload dev does it).

I just tested this in combination with xautoload, and heureka, it does the job - locally.
For this test I disabled the xautoload class loading for test classes, so it really had to use the class loading from the patch, while xautoload did the hook_simpletest_alter().

Btw, the calls to simpletest_classloader_register() are all taken from the original patch in the topic starter. I did not really think much about it, I just hope that these are the only places where we need to call this.

----

There might still be some details missing for testbot / pifr.
We definitely should commit #20. And maybe we need some other things.
But even if it is still insufficient for PIFR, I am pretty sure it is a good first step.

Unfortunately I am scared by the setup instructions for PIFR, so I hope someone else can step in to test this.

#29: 7.x-psr-0-test-classes-autoload-1693398-29.patch queued for re-testing.

EDIT:
The message was "The testbot client is probably malfunctioning."
So I assume this is not caused by the patch. This is why I re-test.

Yessss!!
I would suggest that we get this in, after
- the inevitable code style discussion.
- maybe some test? I don't really know, it is kinda difficult for a testing module to test itself..

Afterwards we can discuss if we add some discovery logic as a follow-up, or if we leave that to contrib.
I think it is a good idea to do this piecemeal. The patch in #29 is easy to review, and the next one will be too.

In the end, if we add the discovery in core, we will have something very similar to the topic starter, only without the dependency on a contrib module.

PIFR already does discover PSR-0 test classes in D6 and D7, it just can't autoload them

I might be wrong on this one.
There is some discovery logic in PIFR that would find those tests, but maybe they will be discarded again if simpletest itself denies them.
Nevertheless I think this can be done in a follow-up.

1., 2. I am quite confident about the logic of the classloader. There is no magic about PSR-0.
If we don't want to rely on this, we could factor out the class finding from the class loading, and do some unit testing with differently shaped classes.
In fact this is what I did when I developed the patch. I just wanted to keep the size of the patch to a minimum, and not unnecessarily pollute the function namespace.

3., 4., 5. My idea was to do that in a follow-up (same issue, but after the class loading is committed).
The class loading in itself is quite useless, but I also expect it to have no unpleasant side effects, so this is why I suggested to do it piecemeal.

leaving out the prefix parts

Some underscore logic is part of PSR-0 spec, and is fully supported by the patch in #29.

Notice the underscores in namespaces and class name:
new Drupal\my_module\Tests\Ab_Cd\Ef_Gh
-> sites/all/modules/custom/my_module/lib/Drupal/my_module/Tests/Ab_Cd/Ef/Gh.php

(EDIT: Maybe not exactly what you were talking about..)

If we want to keep the patch and code nice and small, we could also just unit-test a copy of the class loading function. This is a bit cheating, but I think it would tell us the main thing that we need to know.

Re #0

    $class = str_replace('\\', '_', get_class($this));

Without this, the backslash gets turned into a %5C. I don't see any harm with that.
On the other hand, replacing it with an underscore could, in theory, result in a nameclash. And you can no longer see the url from the classname.
(I only tested with regular manual test runs, no testbot)

Is this taken from D8?
Imo we don't need to replace anything. Or if we do, we should choose something other than an underscore. Unless this is only to be in line with D8..

#38 (myself)
and... I am wrong!
The %5C replacement only happens in the url, but not in the filename. This works in Linux, but will break in Windows.
(this is also why I haven't seen it explode in any of my own tests)

Should we simply replace by dash instead of underscore?

Combination of all patches in this issue: #0 + #20 + #29.
Using the class loading from #29.

#38/#39: Backslash now replaced with dash instead of underscore.

#40: drupal-7.x-psr-0-test-classes-full-1693398-40.patch queued for re-testing.

EDIT: The same test does not fail on my local install. So I re-test.

I knew it :/ don't trust the testbot.

New patch:
- tests added. This will test discovery of various PSR-0 patterns.
- modifed discovery logic to also find class names with underscores. (*)

(*) This is a bit complex, because the conversion filename -> classname is ambiguous in PSR-0.
E.g.
filename: Drupal/simpletest/Tests/Ab/Cd/Ef.php
candidate 1: Drupal\simpletest\Tests\Ab\Cd\Ef
candidate 2: Drupal\simpletest\Tests\Ab\Cd_Ef
candidate 3: Drupal\simpletest\Tests\Ab_Cd_Ef

The new code will include the file and then do a class_exists($class, FALSE) for all 3 candidates to find the correct one.
(the file would be included anyway later in the request to call the static method, so we don't add memory cost.)
Note that we pass FALSE as the second param to class_exists(), so we don't unnecessarily trigger the class loader.

This one should prove that the autoload works.
(during discovery we have the require_once which already includes the file. But on the next request where the test is run, we need the class loading.)

And, guess what, if I disable the autoload mechanic, the test fails.

Note: All the tests need some cosmetic work. Please give some feedback, I am bad at this!

Updated issue summary.

Btw, all the strrpos() and substr() might look ugly, but they are really the most efficient way to do all this string magic.

Question about the tests:
Should we attempt to backport some tests from D8 instead of writing new ones?

And another question:
Will it work on Windows if we assume "/" everywhere, and don't use the DIRECTORY_SEPARATOR constant?
How can we test this?

Forget #44.
The change I announced in #44 didn't make it in there. Did the wrong diff.
This should be the real one.

(probably will still fail like the other two - no idea why)

Ok, now I'm lost.
I know far too little about PIFR.
Can anyone help?

Problem might be related to the underscore magic. Maybe testbot is not prepared for that.

EDIT: Again, wrong diff :(

Reading the error message helps (find-in-page for "Fatal error"):
Class ... not found in .../run-tests.sh

We need to register the class loader in run-tests.sh.

This one is
- without the underscore test (we can re-add it later)
- with a simpletest_classloader_register() in run-tests.sh.

I have not studied run-tests.sh and testbot enough to know if simpletest itself is supposed to be enabled or not when run-tests.sh runs. We'll see if it works.

#56 (sun)
To clarify (for others), we are talking about namespaced classes with underscores in the classname, that is, after the last namespace separator.
PSR-0 says that those underscores should turn into directory separators - whereas underscores in the namespace fragments should not turn into directory separators. See #43.

I personally like those underscores. They allow to have classes in subdirectories, without adding a bazillion of sub-namespaces.
I think I'm add odds with D8 conventions here.. so maybe I should get rid of this habit.
But I wouldn't say that those classes with underscores are not "properly namespaced". They are namespaced, and they follow the spec, so they are.

-----------

One way or another, I want to try if we can support them (and I think we can), with the following patch.
Whether we end up doing that or not, also depends on how it's done in D8 and in PIFR. I will have a look later.

Whether we end up doing that or not, also depends on how it's done in D8 and in PIFR. I will have a look later.

Conclusion:

• PIFR only discovers files, not classes. For class discovery, it depends on simpletest_test_get_all().
• D8 is a moving target. But, it does currently not discover test classes with underscore in the classname.
  http://api.drupal.org/api/drupal/core%21modules%21simpletest%21simpletes...
  http://drupalcode.org/project/drupal.git/blob/d3069f0e61dbc9e14b0b9bd3fa...
  It would be easy to add the underscore discovery logic in D8, but I dunno if anyone wants that.

EDIT:
I would still want the classloader logic to be "spec-complete". And we can only test this if we also have a spec-complete discovery logic.

#59
The class loading logic as in #29 is simple enough imo. It is a solid and complete implementation of the spec, and at the same time it is a lot simpler (and also faster, we can assume) than Symfony or Composer, because it operates on a known and constrained namespace map.
Moving away from that for supposed simplicity (which might save us not more than two or three lines of code) does not sound like a good idea to me.

Removing the test and discovery logic for the underscore case, that's a different story.
At least as long as D8 has no plans to support this stuff.

It's a horrible stop-gap fix either way.

This sounds overly pessimistic.
Which flaws do you see with our approach?

Side note:
The underscore ambiguity of PSR-0 can crash the class loader in D8.

  // Put this anywhere in D8.
  class_exists('Drupal\menu\Tests\MenuTest');
  class_exists('Drupal\menu\Tests_MenuTest');

This will trigger a duplicate file inclusion (and then just explode), because the class loader uses include instead of include_once.
Of course this can only happen if something actually does random and brainless class_exists() or similar.

The underscore-sensitive discovery logic of the patch in #57 walks around this pitfall, by doing a require_once (include_once might be better) and then class_exists($class, FALSE) to skip any duplicate file inclusion.
Besides, the class loader now uses require_once instead of include (and again, include_once might be the better choice.

-----------

We can decide that underscore is totally ruled out, but then I won't just put my head in the sand, but do something reliable. That is, not include the file if the classname has an underscore.
I might do this in the next patch.

Just stick to PSR-0 and PEAR original spec. Either the class name contains _ but no \ then it's a PEAR convention (i.e. My_Namespace_MyClass) or it carries \ then it's PSR-0 (e.g. My\Namespace\MyClass where My\Namespace\My_Class would end up in the My_Class.php file). I might mistaking here but it seems you were mixing both, it is a *really bad* idea. The idea behind a convention is to make everything predictible, mixing both you are breaking developer's minds trying to understant. Keep it simple: drop the underscore. Sorry wrongly read the PSR-0 spec.

@pounard, what you suggest is not PSR-0. Please read the spec, esp the part about underscores:
https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-0.md#u...

I don't understand why you all find this so horribly complex.

Sorry, didn't see that they actually do allow _ in class names to be folders, I guess that's for PEAR compatibility.

Intermediate summary:

• I will post an alternative patch soon that will responsibly disallow the underscore in classnames, without breaking the PSR-0 spec in the class loader. This will go along with an updated patch that does support the underscore, so we can compare them side by side.
• Future patches will have PHP version checks, so we don't break PHP 5.2 compatibility.
  Any idea how we can test PHP 5.2 compatibility?
• There are a few places where we have to choose between require, include, require_once, and include_once. I am going to make some choices and explain the rationale behind that.
  (Symfony / D8 class loader uses include, because it assumes we never hit a file twice. I am inclined to do the same.)
• #47: Question: Should we attempt to backport some tests from D8 instead of writing new ones?
• #48: Question: Will it work on Windows if we assume "/" everywhere, and don't use the DIRECTORY_SEPARATOR constant?
  How can we test this? Any volunteers on Windows? Or can this be automated?
• We need overall cosmetic improvements and review. I am already working to add some comments, but I need more eyes.
• Reviewing might be easier if we split up the patch. E.g. one step for the class loading, another for the discovery, and a third one for the tests.
  Any idea how we can do this, if the latter patches depend on the first one?
• I still want to know why "It's a horrible stop-gap fix either way."

Actually, I originally misread the PSR-0 spec, but it's clear now and I agree with donxiquote. Sorry once again for the noise.

Now about this issue, more than once in work related projects I needed working PSR-0 autoloaders. I also needed it in contrib modules, and always implement very bad hacks in the module file headers to add last resort autoloaders in case classes cannot be found. This is all of ugly, unpleasant, and unperformant.

I, as a lot of other people I think, need a decent and fast PSR-0 autoloader in core for:

• Simpletest? Why not.
• External libraries (very important)
• Some modules that embeds PSR-0 code.

So here is my small brick for building this wall:

• I think we can build it custom without using Symfony, so that it will work with PHP 5.2. It is relatively easy to build a PSR-0 autoloader.
• We need a settings.php variable for registering namespaces. E.g. $conf['autoloader']['MyNamespace'] = '/some/dir';
• We need two functions, one for registering a new namespace and the other for deregistering it (always provide the counterpart).
• There is no need to check for PHP 5.2 version if the autoloader itself runs in PHP 5.2, because it will just never have classes with '\' insider, and will autoload legacy PSR-0 / PEAR classes without breaking. That's why we absolutely should not try to embed Symfony loaders. Writing a PSR-0 loader is easy let's just do that!
• We need to provide a decent interface for autoloader front so we can (or contrib can) replace it seamlessly with either a map based or an APC driven implementation.

Now some answers:

Future patches will have PHP version checks, so we don't break PHP 5.2 compatibility.
Any idea how we can test PHP 5.2 compatibility?

I think this is not necessary. As said upper if the autoloader is written in PHP 5.2 it work gracefully in both PHP 5.3 and PHP 5.2. It's just doing strings operations (e.g. strpos($className, '\\') for example). The only place where it'd be necessary will be in unit tests only, PHP 5.3+ tests cannot be run therefore must be skipped when test suite is ran on PHP 5.2 boxes.

There are a few places where we have to choose between require, include, require_once, and include_once. I am going to make some choices and explain the rationale behind that.
(Symfony / D8 class loader uses include, because it assumes we never hit a file twice. I am inclined to do the same.)

Best choices are always _once alternative, but because classes are supposed to be autoloaded only once it's not necessary. The include instead of require alternative also allow the code to never fail if a file cannot be loaded (the loader might fail and another chained one could succeed for the same class). So yes I agree, just go for include I think that's fine.

Question: Should we attempt to backport some tests from D8 instead of writing new ones?

Tests in D8 are written in PHP 5.3 I guess? I'm not sure this is revelant to backport them.

Question: Will it work on Windows if we assume "/" everywhere, and don't use the DIRECTORY_SEPARATOR constant?

Windows is not that stupid, it works gracefully with "/". I actually worked a few monthes recently on windows boxes and I did tricky stuff with autoloaders on custom projects and everything went fine. PHP in windows converts transparently every "/" in "\" for the file system calls.

You actually changed the topic at #3

This would be insane to separate both, the need is the exact same, and the code would probably be too.

Because we have a real need for both, tests and contrib, it surely would be a great thing to improve both! I'm ready to help, review, and make some patch for this to move.

@pounard,
PSR-0 support in D7 contrib (xautoload, classloader) does work quite well.
Simpletest (+PIFR) is currently the only case where a contrib class loader does not cut it, and here it is perfectly ok to use a very simple class loader constrained to the task.

This would be insane to separate both, the need is the exact same, and the code would probably be too.

I would normally agree for reuse / synergy / performance.
But in this case, the synergy is very small, and I suspect that performance would actually decrease instead of increase.

------

The one argument in favor of a generic class loader in D7 core would be that existing modules can start using PSR-0 without adding an additional dependency.
Right now those modules can write a simple fallback autoloader similar to the patch we discuss here. This does actually work quite well, but it is an extra effort that some might want to avoid.

If we ever get a PSR-0 loader in D7 core, we can re-evaluate if it should be used by Simpletest.
For now I would much prefer to follow the direction of the currently discussed patch.

Fallback is not the right path, and cannot work in some cases (and contrib too) for example when you need the autoloader for loading a cache backend. That's why a core loader with a settings.php configuration would be the best bet for this to work without hacking (the worst hack I had to wrote is adding a cache include file to register autoloaders).

In the redis module for example, the most complex part is the autoloader fallback, and that's very sad.

But, I'll open another bug then.

Just for the sake of testing, here is a patch.

#71: 1693398-71-psr0-d7.patch queued for re-testing.

Opened #1902894: Add a generic PSR-0 autoloader to Drupal 7 to resolve dependencies

@pounard (#71),
could you elaborate what you did differently in your patch, and why?

I still would prefer a minimal, procedural, non-pluggable class loader for this particular purpose. For the sake of a small patch footprint.
And I would prefer not to wait for a generic class loader in D7 core. I have modules waiting that need test coverage. (and I think others have too)

My patch introduces a class instead of procedural code for autoloading, and doesn't do anything automatically. It also ships an APC front implementation that is automatically enabled if APC is found. It ships a set of methods for registering and unregistering namespaces, which allows the users to use it to their will. It also provides a simple settings.php variable for registering namespaces / include pathes couples in configuraiton. Finally, the autoloader is inited in drupal configuration boostrap phase, which makes it being up and ready long before cache and database, allowing very early code to rely on PSR-0 code without needing anything else to be set up than the namespace in settings.php file, which allows, for example, cache backends or hypothetical database drivers to use PSR-0 code or to rely upon external PSR-0 or PEAR libraries. As a downside, it does not auto register anything, leaving core untainted, it does not play with simple tests. If simpletest needs it, then you'd still need auto to register modules yourselves. Once again, it aims easiness for embedding external libraries more than adding features to core itself. Drupal 7 is not a candidate for new features, so my patch doesn't bring one, it just gives a unified autoloader for contrib code in order to get rid of all those fallback code blocks, monkey patches, and unnecessary dependencies we experience in a lot of contrib modules (apachesolr, search_api_solr, redis, and probably a lot more).

Isn't this all pretty much overkill for this issue?
None of the features you mention are necessary for test classes.

I propose:

1. A simple, procedural, hardwired, non-pluggable, easy-to-review, small-patch-footprint class loader for test classes, to go into D7 core (simpletest module) asap.
   It should respect the PSR-0 spec, but it doesn't need to be flexible in any way.
   See also #34.
2. A discussion about a pluggable class loader in D7 core to happen in a separate issue.
3. If and when this other class loader should ever land in D7, we can reconsider to also use it for test classes.

This can happen yes, I'm not against this methodology. I'm not against this patch, but by joining forces altogether we could make it right quicker than by spreading patches loosing the real important thing in the end: having a unified autoloader. Right now there is absolutely no use for using PSR-0 code only in tests, most tests actually exists using the files[] core autoloader, and work fine, whereas a lot of external libraries are already in use since a few years by a lot of contrib modules on thousands of sites, and those have a real need for performances, unification of configuration, flexibility in site building and easiness of deployement.

I would love to see this landing asap, as I am getting blocked here for d7: http://drupal.org/project/testing.

@Sylvain,
me too.

Can you give some feedback on the patch in #57 and the summary in #65 ?

To give feedback on #57:
===================

On simpletest.module, line 333:
$tests_dir = DRUPAL_ROOT . '/' . dirname($filename) . '/lib/Drupal/' . $name . '/Tests';

This is nice to prefer convention over configuration, however, I think this convention don't enforce best practices. I think test code should be separated from the production code, why not introducing a test directory ? @see #1872006: Add parity test classes for lib/Core and lib/Component. This would also helps developers moving from non-namespaced tests to namespaced test by the separation of directories.

lib/
  Drupal/
    module/
      Class1.php
      Class2.php
test/
  Drupal/
    module/
      Tests/
        Class1Test.php
        Class2Test.php

Same remark for line 424: $path = dirname($extensions[$extension]) . '/lib/' ..

/**
 * Run selected tests.
 */
function simpletest_test_form_submit($form, &$form_state) {
  simpletest_classloader_register();
  // Get list of tests.
  $tests_list = array();
  simpletest_classloader_register();

In simpletest.pages.inc, There is line 184 and 187 the same function called twice, I guess this is a mistake ?

To give feedback on #71:
===================

I quite like the approach in this patch, but all the glue code to register modules tests is missing. e.g. the foreach($modules as $module) { drupal_register_namespace($module); }.

The drupal_autoload_get() provides a static configuration, meaning its actually not really plug-able, unless you do drupal_static_set($my_loader); The configuration of the loader used depend on the $conf['apc_classloader_enable'], and the fact that the APC extension is loaded. This can be optimized later (Drupal 8) so that the configuration get compiled in the DIC and you never ever have to execute this "class factory" procedural code.

Conclusions:
=========

The former patch (#57) ensure convention over configuration, it is a good practice but we should improve the convention, move tests classes in a separate directory than the production classes. e.g. having lib/ and test/ directory.

The approach taken in the later patch (#71) is missing glue code to actually map and find the tests, also missing integration (web based) tests. The static configuration of the class loader is good as a temporary solution, e.g. for Drupal 7. It can be replaced by a solution completely managed by a DIC in Drupal 8.

we should improve the convention, move tests classes in a separate directory than the production classes. e.g. having lib/ and test/ directory.

Interesting. I have some quick thoughts which convention I might prefer, but this should be discussed in the other issue.
For this issue, whatever convention is decided for D8 should also be the one we use in D7.

The only thing that worries me is that this discussion will further slow us down, because now we have to wait for the bikeshed to be expected in the D8 issue :(

same function called twice, I guess this is a mistake ?

Whoops. Yep.

Yes you are right, this convention as you mentioned in #1872006: Add parity test classes for lib/Core and lib/Component has some flaws, specially for modules which have already a tests directory and for Integration tests (Web based). However this is always interesting to separate production code from testing code physically. This does not belong to this issue but I wanted to highlight it. We can improve it later.

I am generally OK with the patch in #57, why do this issue is set as needs work ?

It is an improvement for Drupal 7 because:

• For "Simpletest" enabled websites, Registry wont be bloated anymore by testing classes if using PSR-0 test classes. We will be able to remove the file[] declaration from .info files for websites using PSR-0 tests under PHP5.3. This will impact positively these websites.
• It does not need a perfect autoloader because the classloader module will fulfill the needing for PSR-0 classes.
• I like the work pounard did, but again classloader module already doing it.

On a lower level and more detailed review:

I really don't care about the implementation as it will be only executed in a testing environment, so the quality of algorithms (don't take it as a critic) really don't matter. Secondly it will be replaced in Drupal 8 by autoloader component, and in Drupal 8/9 by PHPUnit, so it is perfectly acceptable and you provided three integration tests. I guess the underscore problem can be fixed in a follow-up (I am not using underscore in my classes so for me its not a big deal).

For PHP5.2 compatibility, I think we don't care; for me this is perfectly acceptable:

    // Tests within disabled modules.
    if (version_compare(PHP_VERSION, '5.3') < 0) {
      // Don't expect PSR-0 tests to be discovered on older PHP versions.
      return;
    }

As promised in #65, here is one version with underscore-classes being discovered, and another where those classes are ignored.
The class loader is the same for both.

I guess the underscore problem can be fixed in a follow-up (I am not using underscore in my classes so for me its not a big deal).

Imo it is a simple decision, no follow-up necessary.
Either we discover them, or we ignore them.

I had a quick look, D8 ignores the underscore problem altogether.
It always assumes that the file defines a class with no underscore after the last namespace separator. It will probably crash if this is not the case. (which is ok, module developers simply need to follow the conventions)

Both patches in #85 are more robust than D8 in this regard.
This does make sense if you consider we throw this into a living ecosystem, not knowing which modules already have test classes in their /lib/ folders and how these look like.

Heck, why does it work locally then?
Testbot has failed me before in this issue :(
Could this be something we need to worry about?

Ouch!
The previous patches had some "<" instead of ">" for the version compare.
The only reason it didn't fail for me locally was because I had not refreshed my cache..

Yay, green!

Follow-up to the intermediate summary (#65)

Done:

• Alternative patches with and without underscore.
• PHP version checks where they are needed.
• Using *include* in the class loader, *include_once* in the discovery. In both cases we are pretty sure that the file exists.

Questions remaining from #65:

• Can and should we test PHP 5.2 compatibility?
• #47: Question: Should we attempt to backport some tests from D8 instead of writing new ones?
• #48: Question: Will it work on Windows if we assume "/" everywhere, and don't use the DIRECTORY_SEPARATOR constant? How can we test this? Any volunteers on Windows? Or can this be automated?
• Cosmetic improvements and review?
• #59: Why is it a "horrible stop-gap fix" .. whatever.

New questions:

• How to deal with the underscore, finally? The second option is the most consistent with D8 - although D8 is a moving target, isn't it...
• Where to put the tests? /lib/ or /tests/ dir?
  #1872006: Add parity test classes for lib/Core and lib/Component

Ok, just to clarify something, unlike Drupal 8 where you can provide your own namespace for a given module, here you cannot. It is assumed (convention) that the namespace should be Drupal/$module_name/Tests.

Can you confirm this is by design ?

Also, going into the low-level of the patch, you should change:

<?php
      $files = file_scan_directory($tests_dir, '/.*\.php/');
?>

to:

<?php
      $files = file_scan_directory($tests_dir, '/.*\Test.php/');
?>

This makes crash simpletest_get_form() when it tries to load a ServiceStub.php class which rely on a PSR-0 ServiceInterface which belongs to lib/ package. By restricting to Test.php classes, we ensure only TestCase based classes will be loaded.

You can have a check at http://drupalcode.org/project/wow.git/tree for an implementation of PSR-0 tests. (branch 7.x-3.x).

Drupal 8 where you can provide your own namespace for a given module

I am not aware of this.
By default, Drupal 8 will register any Drupal\$module\ namespace in $module_dir/lib/(Drupal/$module/). This is also what D7 class loading modules do (xautoload, classloader).

There might be possibilities to register additional namespaces, but you get the default ones "for free".
If you have other information, let me know..

#94,
damn, that's a good point.
It could get even worse: In theory, contrib or custom modules could already have put anything they want into the /Tests/ folder, any of this could crash.
But ok, we should not try to fix every possible problem, but focus on the more likely issues.

I am not aware of a policy (for D8) that says that a test class must end with "Test". Afaik, anything inside the /Tests/ folder is considered a test, if it inherits from DrupalTestCase.
In this patch I would like to stick as close as possible to the conventions of D8. (not necessarily the code, but the behavior)
The discovery logic is largely taken from #0, I suppose this would have the same issues.

I might have a solution, but it is kinda freaky..

When using the classloader module to test PSR-0 objects, I have to unregister the drupal_autoload_class and drupal_autoload_interface or it throws warning (checking table {registry} in unit test which does not exists of course) because they are registered before our classloader and simpletest_psr0 functions:

<?php
  protected function setUp() {
    parent::setUp();
    spl_autoload_unregister('drupal_autoload_class');
    spl_autoload_unregister('drupal_autoload_interface');
    drupal_classloader_register('wow', drupal_get_path('module', 'wow'));
  }
?>

It is perfectly possible to have classes which are not test in the Tests namespace, otherwise, where you would define your Mock and Stub object ? With the production classes ? no way !

Because the use case of testing PSR-0 classes is completely valid, I think the initial approach of not backport an auto-loader cannot be considered anymore. We need an unified solution, which will act as an autoloader for either classes and tests classes. Pounard proposed something without backporting the symfony component.

The only way I found to test PSR-0 classes within PSR-0 tests was to manually load the classloader module in the setUp method, then register my module namespace and unregister 'drupal_autoload_class' and 'drupal_autoload_interface' because it checked the DB in a unit testing context which thrown exception saying me the {registry} table could not be found. If this is acceptable from a DX then I am ok with this patch. Otherwise, we need to find a better approach.

There are two issues here.

------------

First, the database warning about {registry} table in unit tests.
Explanation: The registry-based drupal_autoload_class() and drupal_autoload_interface() will crash if called during unit tests.
We can try to use classes that are not based on the registry, but then it still crashes because the registry-based class loaders are first.
I experienced the same when writing the 7.x-3.x of xautoload. The solution was indeed to change the order of class loaders in the spl autoload stack in the setUp() method.
http://drupalcode.org/project/xautoload.git/blob/6a989563bb9c038c35daffa...
Maybe I will come up with something smarter at some point.
I would say that this issue is not really related to PSR-0. I suppose it would be easy to craft a D7-registry-style unit test that would crash because of this.

----------------

For the second thing,

It is perfectly possible to have classes which are not test in the Tests namespace, otherwise, where you would define your Mock and Stub object ? With the production classes ? no way !

I don't see a big problem in having them outside the \Tests\ namespace:

Drupal\$module\.. (production stuff)
Drupal\$module\Tests\..
Drupal\$module\Mock\..
Drupal\$module\Stub\..

I could certainly live with that.

Here is a commit where I refactored RequestTest and CallbackTest classes to PSR-0 classes. Check this out: http://drupalcode.org/project/wow.git/commitdiff/11627c06da5e32c196b6a41....

The files you might be interested in having a look to are:

• lib/Drupal/wow/Tests/RequestTest.php
• lib/Drupal/wow/Tests/UnitTestBase.php
• lib/Drupal/wow/Tests/Response/CallbackTest.php

Because the use case of testing PSR-0 classes is completely valid, I think the initial approach of not backport an auto-loader cannot be considered anymore. We need an unified solution, which will act as an autoloader for either classes and tests classes. Pounard proposed something without backporting the symfony component.

This does not solve any of these problems.
In fact, I expect that your example would crash just as badly in D8:
- Create a D8 module.
- Put a mock class into the Drupal\$module\Tests\ namespace.
- Let it inherit from a class that lives outside of the \Tests\ namespace.
- Disable the module
- Visit the tests overview page.

Yes, this will crash, because
- test classes of disabled classes are discovered and included
- everything in \Tests\ namespace is considered a potential test class
- only enabled modules' root namespaces (Drupal\$module\) are registered in the class loader. For disabled modules, only classes in \Tests\ are available.
-> crash.

The {registry} warning can be fixed like this:
#1916328: Unregister registry-based autoload during unit tests

Ok so mock objects has to live in a separate directory than the test one... This is for me really not acceptable. Anyway that's why I am also working on deploying a PHPUnit system in #1801176: Deploy a PHPUnit system with a bottom-up approach.

I can live with the fact that I need to put mock and stub objects in another yet directory, but it actually pollutes my classes libraries and I don't like it. In the end, because I believe naming things correctly contribute to better readability, I will not use the default namespace layout of Drupal 8:

I will register a namespace "WoW\\Core" for the wow module (instead of the "Drupal\\wow" suggested), then "WoW\\Character" for the character component (instead of "Drupal\\wow_character" - why the heck should I have underscore in my namespaces ?!). And then, for the tests to understand a Drupal\wow\Tests directory, and a Drupal\wow\Mock directory.

I am not happy but generally ok with this, for two reasons:

1. I participated intensively to this discussion, so i know the standard directory layout to put my PSR-0 tests to be recognized. But I guess as a new developer I would have been pissed-off by this.
2. In the future, when tests will be PHPUnit ready, I will be able to remove the Mock directory as I will use the Mock API of PHPUnit.

Drupal 7 "Forward-compatible" PSR-0 tests would then look like this:

- lib/
    WoW/ # Module library
      Core/ # Core classes
      Character/ # Character component classes
    Drupal/ # Drupal wrapper
      wow/
        Tests/ # Standard test directory layout
        Mocks/ # Classes that I don't want to be loaded by simpletest because it will simply crash
        Stubs/ # Same as above but for stubs

All my Unit tests will have to disable drupal_autoload_* functions temporarily to avoid warnings.

This is in my opinion a lot to understand for a newcomer, but I can live with it.

I can understand there can be different opinions about the Drupal\$module\ namespace convention. However, this was decided for D8 long ago, and I don't think anyone would want to restart a discussion about it.

You can think of it like this:
- Drupal\www\.. would contain the classes for the wow Drupal module.
- WoW\... would contain classes for a WoW composer package / 3rd party library from github / etc.

There are cases where both of these exist - a 3rd party library, and a Drupal module. The different namespaces help to avoid a clash.

All my Unit tests will have to disable drupal_autoload_* functions temporarily to avoid warnings.

Again, this shall be fixed in #1916328: Unregister registry-based autoload during unit tests

This issue gave me a nice idea: #1916410: DX: Allow programmer to specify a different namespace (a.k.a classes under lib/ folder) in .info files..

Given the fact you gave me a work around for my mocks object, given the fact you opened #1916328: Unregister registry-based autoload during unit tests, and given the fact tests are working for me, I can set this as RTBC.

The discovery and class validation code needs to be identical to the code in D8

So the idea is that we attempt to be identical to D8 unless there is a good reason not to?
Fair enough.

We cannot or might not want to be 100% identical because
1. In D7 we also need to discover registry-based test classes, which is not the case for D8.
2. For D7 we throw our changes into a living ecosystem, so any "you are not allowed to do that" that we can expect in D8, may already have been trampled over in D7 contrib. E.g. there could be all kinds of junk already in the /lib/ folders of D7 contrib modules. So we may want to be more robust in D7 than we are in D8.

For 1., I found it very helpful to split out the psr0 discovery into a separate function, and then do an array_merge(). I hope you are not opposed to that idea?

For 2., I think the worst we can realistically expect is a FATAL error whenever tests are discovered, because D8 discovery logic (and the D7 patch) will include whatever it finds in the /Tests/ folders. Typically this will happen if classes within that folder depend on stuff that is currently not available due to the module being disabled. The other would be the underscore issue.
We can account for some of these cases, but we cannot entirely avoid it. Unfortunately, we cannot try/catch a FATAL.

So for some sites it means that tests no longer work, until such modules are fixed.
I guess we can live with that?

The patch attempts to prevent the underscore issue, but it does not prevent any other issue that could be caused by file inclusion, esp, the possible dependency on disabled modules. So, fine with me, we might also remove the underscore logic, and try to be as identical to D8 as possible.

Btw, one way to avoid the possible inclusion error would be that we introduce a setting, that needs to be explicitly enabled to activate PSR-0 tests.
Not sure what this would mean for testbot?

Stale copied code.

Ok. Need better comments on those test classes.
Do you agree with the "arithmetics test" as an example for a test that always passes?

+++ b/modules/simpletest/drupal_web_test_case.php
@@ -447,7 +447,8 @@ abstract class DrupalTestCase {
+      $class_safe = str_replace('\\', '-', get_class($this));

Derives from the D8 code / #0, which at minimum uses underscores instead of hyphens.

Motivation for the dash hyphen is in #38, #39.
Btw, this is a different underscore issue than the one i was talking about elsewhere. This one can be caused by underscores in the namespace, not just those in the class name.

So, our policy is, if something is broken (or fragile) in D8, then we cannot allow it to be any less fragile in D7?
Assuming we would decide to replace the underscore with a hyphen on D8, do you expect we would ever backport this change to D7?

#109:
Quick answer: I have some objections about your arguments, but I agree with most of the conclusions :)

I don't know from where you assume that something would fatal, but the discovery code actually contains sufficient conditions to prevent false-positive matches and to ensure that all found classes are actually test classes. That concern is completely bogus IMHO.

The way these conditions work is by doing class_exists(), which will indirectly but inevitably include all those files. If any of these files contains something fishy, we may get a FATAL or whatever there is.

Sylvain in #97 does run into this exact issue (among others).
I have provided some explanation in #98.
I could go further, but I think it is better for this issue if I don't.. I might open a new issue, if only for reference and to be "wontfix" immediately, but I'm not sure this is the best way to do this.

The reasons why I finally agree we should not care about these theoretical issues:

• It will only ever crash admin/config/development/testing. It won't affect any other pages. This does give people time to fix their modules, in the rare case that anything breaks.
• The check I introduced in the discovery code was designed to fix an issue we no longer have, I realized. They do not actually prevent any of the remaining issues.
• Beyond this sanity check (which does not add any benefit) there is very little we can do. We bite the apple or we don't.
• As you already mentioned, those issues can be expected to be quite rare or unlikely. In combination with the other points, this indeed means we can "take the risk" - even though I generally tend to be careful with assumptions.

Another attempt.

Removing the underscore stuff altogether.
It is still in the class loader cause that's the spec, but it is removed from the examples.

Some further changes to make it more like #0 / D8.

The "-113-vs-0.patch" shows the difference to #0
The "-113.patch" is the actual patch.

This work for me :-)

I thought we weren't using 'We' in drupal documentation, but I guess this is not a blocker.

I need other people to tell me this stuff :)
And ideally suggest something different.

Great work here. I found a lot of nitpicks while reviewing this (and also in general felt that this patch could use one or two more final reviews from others). However, it seems like a really nice feature to get into the next Drupal 7 release, and because it's Simpletest (rather than a module that runs on production sites) we can be more lenient, so I decided to go ahead and slip it in.

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/7cbe4b7

However, it would be great if we could follow up with some code cleanup here. Here are some things I noticed, but as mentioned this could use another reviewer as well:

1. +      'name' => 'PSR0 web test',
   +      'description' => 'We want to assert that this PSR-0 test case is being discovered.',
   +      'group' => 'SimpleTest',
   +    );
   +  }
   +
   +  function testArithmetics() {
   

   (a) Use "PSR-0" not "PSR0" (throughout the tests).
   (b) It's OK to use "We" in code comments but not in the user interface... How about "Tests the discovery of PSR-0 test classes."? And then, add documentation to testArithmetics() that says something more like "This test exists to check that this PSR-0 test case can be discovered by the autoloader."
   (c) testArithmetics() should be testArithmetic().

2. +/*
   + * Register a simple class loader that can find D8-style PSR-0 test classes.
   

   (a) The first line should be "/**".
   (b) Probably change "D8" to "Drupal 8" (or just leave "D8-style" off altogether).

3. + * This will only work on classes where the namespace is of the pattern
   + *   "Drupal\$extension\Tests\.."
   

   No reason for the extra indentation on the second line.

4. +        $nspos = strrpos($class, '\\');
   

   In general we don't abbreviate variable names. How about $namespace_position?

5. +/**
   + * Verifies that tests in other installation profiles are not found.
   + *
   + * @see SimpleTestInstallationProfileModuleTestsTestCase
   + */
   +class SimpleTestDiscoveryTestCase extends DrupalWebTestCase {
   +  /**
   +   * Use the Testing profile.
   +   *
   +   * The Testing profile contains drupal_system_listing_compatible_test.test,
   +   * which attempts to:
   +   * - run tests using the Minimal profile (which does not contain the
   +   *   drupal_system_listing_compatible_test.module)
   +   * - but still install the drupal_system_listing_compatible_test.module
   +   *   contained in the Testing profile.
   +   *
   +   * @see DrupalSystemListingCompatibleTestCase
   +   */
   +  protected $profile = 'testing';
   

   This whole thing looks like a bad copy-paste? The first line ("Verifies that tests...") is not describing what this class does. The @see looks unrelated, and the in-depth documentation of the protected $profile = 'testing' line isn't relevant and could be removed here.

6. +    // TODO: What if we have cached values? Do we need to force a cache refresh?
   +    $classes_all = simpletest_test_get_all();
   

   I don't know, but let's figure it out and remove the comment :)

7. +      $this->assert(!empty($classes_all['SimpleTest'][$class]), t('Class @class must be discovered by simpletest_test_get_all().', array('@class' => $class)));
   

   This is true in a lot of other places in the patch but I'm highlighting it here:

   (a) Use $this->assertTrue() rather than $this->assert().
   (b) Don't use t() in assertion messages. Use format_string() in cases that have variable substitution (like the above) or a regular string otherwise.
   (c) The wording reads a little odd to me... I don't think "must" is the correct English word. I think just "Class @class was discovered by simpletest_test_get_all()." would be more in line with the wording in most assertion messages. (Similar for other uses of "must" throughout the patch.)

8. +    // Tests within enabled modules.
   +    // (without these, this test wouldn't happen in the first place, so this is
   +    // a bit pointless. We still run it for proof-of-concept.)
   +    // This one is defined in system module.
   +    $this->assertText('Drupal error handlers');
   +    // This one is defined in simpletest module.
   +    $this->assertText('Discovery of test classes');
   

   Like the code comment says, I don't see the point of these :) It's also kind of fragile. The tests later on in the function (which look for the specific classes we're looking for in this test) seem more than sufficient to me.

   Can we remove them?

9. --- /dev/null
   +++ b/modules/simpletest/tests/psr_0_test/psr_0_test.info
   @@ -0,0 +1,6 @@
   +name = PSR-0 Test cases
   +description = Test classes to be discovered by simpletest.
   +core = 7.x
   +
   +hidden = TRUE
   +package = Testing
   

   This is missing the standard "version = VERSION" and is also in a bit of a funny order. I'd suggest just copying from an existing test module and using the same format.

Thanks a lot for the review!
I am a rookie on style and "best practice".
Will try a follow-up patch when I have time.

This is my understanding of the current situation. Please correct me if I'm wrong.
D7 run-tests.sh and Simpletest UI would not discover D8 style PSR-0 tests. After #1674290: PSR-0 tests in contrib modules are not found went in, the classes would be found initially but any namespaced class would not make it into the class registry, thus would not be considered valid and was filtered out of the list.
As of now (Drupal 7.22) we can run D8 style PSR-0 tests in D7 modules without resorting to ugly hacks.

That's it. Right?

Then I guess the issue summary could do with some updating/ simplifying. And don't we need a change notice for such a wonderful change?

Here is a follow-up fixing some comments in #116.

1/ I removed the arithmetic test, although very simple, there is no reason to do some arithmetic when we just want to test an auto loader capability. Thus I changed it to:

<?php
  /**
   * Asserts the test is found by autolaoders.
   */
  function testAssert() {
    $this->pass('PSR-0 test loaded and running.', 'SimpleTest');
  }
?>

2/ Corrected.

3/ Corrected.

4/ Changed to $namespace_pos.

5/ Corrected documentation of the class and removed in-depth doc block for profile.

6/ I don't know either, but i've applied the drupal static cache pattern for this function. I think its a good practice.

7/ Changed assert to assertTrue, used format_string instead of t(), changed the wording 'must' to 'has been discovered'.

8/ I removed those tests.

9/ Changed with correct order and version tag.

Needs review of course :-)

Review this patch better.

I have no objection with any of these changes.
For my taste, #121 can be committed, and then we can optionally do another follow-up round.

I won't set this RTBC. I leave this to others more experienced with code style reviews.

Hey its August nearly, maybe this need some love again ?

According to donquixote, let's commit this.

Overall this looks great to me! - the only issue I see is with the $reset parameter to simpletest_test_get_all(). That's adding a new function parameter in Drupal 7 which is then "removed" in Drupal 8, which is always a little messy, and I looked a little closer at the code and don't think there's any need for it; Simpletest already calls drupal_static_reset() in the setup method of every single test run, which will reset the static here along with all others throughout the Drupal site, right before this test runs. So I think the TODO could simply be removed instead...

Overview of different approaches and their problems.

I had completely forgotten that this issue existed. I just created #2293767: Allow PSR-4 test classes to be used in D7. I hope I will get to fixing this issue up in the next days/week.

The docs below say that this issue should be closed because it is already supported ...?
https://www.drupal.org/docs/7/testing/organizing-your-test-cases

Are the docs wrong and should be updated or should this issue be closed?

I've got PSR-4 autoloading working with a standard Drupal 7 site (see https://www.drupal.org/project/drupal/issues/2293767#comment-13432531).

I'm happy to look into updating the documentation.