You are here

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

Problem

Goal

Add optional support for PSR-0 tests to Simpletest in D7.

All in contrib? Nope, doesn't work.

The first idea would be to do it all in contrib:
- Use a contrib PSR-0 class loader, e.g. classloader module or xautoload module.
- Write a contrib module that can discover the PSR-0 test classes, and that will register them with hook_simpletest_alter().

This approach does work locally, if the admin makes sure that the respective contrib modules are enabled.
However, it does not work with testbot / PIFR for the automated tests on drupal.org. Why?
- testbot does not install any contrib modules prior to test discovery. (and apparently we cannot force it to do so, or noone really wants that)
- pifr has a custom discovery mechanism which ignores hook_simpletest_alter().

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.

No, the full class loader cannot be backported to D7. Especially, because it is still in flux; e.g., see #1663404: Use Composer's defined namespaces to ease maintenance. Furthermore, that would require to add a /vendor directory at the top-level (since D7 does not have a /core directory), etc.pp. So no, definitely not possible.

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.

@Rob Loach: Not in core, obviously. But I'm actively working on various contrib modules with the goal of moving them into core for D8. For those, I'd love to use the PSR-0 structure in D7 already, which the classloader/autoload module allows. The only missing piece is enabling the PSR-0 tests. (example)

#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?

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.

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.

@donquixote: I've the impression that you didn't spend a minute to look at the actual, proposed patch. 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. Additionally, 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.

However, that's a helpful pointer by itself, invalidating the approach taken by this patch, since there cannot be a drupal_classloader_register() function unless such a function has been loaded, so the code path beyond the function_exists() is never executed.

So back to the drawing board. We need to load code that may not necessarily exist, and if it exists, then it is contained in a module of which we, off-hand, don't know the location.

This might actually turn out to be a good exercise for Drupal 8 and beyond, too. OTOH, it might be easier to get Composer onto testbots, and instead of some of the changes in the current patch, we add a simple composer.json to D7 with a 'require-dev' and specify symfony/class-loader there. Thus, all run-tests.sh would have to check for is ./vendor/symfony/class-loader/..., include it, and register the namespaces of all extensions before executing any command/operation. Hm.

@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.

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.

@donquixote:
The blatant thing you are missing is that your module is not enabled in the first place. You enabled your module in your Drupal site. You have to disable and uninstall it. Then, run-tests.sh, and either see how it blows up, or how all of your PSR-0 tests are missing.

I thought this was clear from #18 and everything else already. If this is still not sufficient, then I recommend to study the code base of PIFR a bit.

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.

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.

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.

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.

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.

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.

Introducing a bare minimal, scope-limited PSR-0 autoloader that only works for test classes, and only within Simpletest, works for me.

The autoloading code logic/implementation has to be bullet-proof though, since we definitely do not want to introduce new bugs in D7 at this point. ;) Ideally, we might want to consider to copy/paste the FQCN-to-filename conversion code from UniversalClassLoader (of course, leaving out the prefix parts, but they're cleanly separated in there anyway).

We're still missing the required DrupalTestCase changes from #0 though.

I think we also need to take over the test class discovery from #0, since D7's regular classloader (The Registry™) isn't able to cope with them, and is also not designed to handle tons of registered classes (it's designed for a few files containing multiple classes), so you'll probably notice a performance difference due to the massive amount of additionally registered files/classes if they were registered through .info files (at minimum during registry rebuilding).

With this approach, we can and thus we should add an actual PSR-0 test class to D7, and make Simpletest test itself. Simpletest is able to do that by setting up a recursive child-testing site. We're doing that for other Simpletest tests already.

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.

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.

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)

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.

We should not care for the full PSR-0 spec in the test autoloader for D7. I.e., we simply do not support the underscore conversion in the test autoloader. In the PSR-0 spec, that's only legacy backwards-compatibility support for old-style-PEAR classes in the first place.

For D7 modules, we can and should keep it simple: Only properly namespaced PSR-0 classes are supported. Nothing else.

#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.

#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.

<?php// 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.

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?

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 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.

@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.

@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:

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.

A discussion about a pluggable class loader in D7 core to happen in a separate issue.

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.

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: [PHPUnit] 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.

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 :(

Yes you are right, this convention as you mentioned in #1872006: [PHPUnit] 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:

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.

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.

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.

#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.

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:

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.

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:

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.

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:

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.

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.

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.

Derives from the D8 code / #0, which at minimum uses underscores instead of hyphens. Potentially further differences. We're trying to keep the Simpletest testing framework code as equal and comparable as possible.

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?

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?

The currently proposed discovery code looks more complex than it needs to be; the D8 code is simpler and should work identically for D7. Those few lines should not require a separate function.

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.

Due to that, there's also no need to take any special care for potential matches on other code in $module/lib/Drupal/$module/Tests/ directories. For one, D7 does not support PSR-0 yet, so the chances for such a directory to exist are close to zero. Second, as mentioned before, all matched files are verified before usage.

None of this will affect production sites in the first place, since I've yet to see anyone mentioning that tests are being run on production sites.

Yes, the hyphen/underscore issue was related to the verbose browser/debug output filenames of DrupalTestCase, not the PSR-0 logic.

The D7 code of Simpletest and the testing framework is supposed to be as identical and comparable as possible. Bugs are fixed in HEAD first.

#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.

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.

(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().

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.

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

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.)

+ // 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.

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?

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...