So currently, because of the existence of the DB registry and its precedence over the PSR-0 it is not possible to write unit tests strictly against PSR-0 classes because you will get a database error as the registry tries to find the file. The eventual fix is better management of the registry or hopefully its removal but here's a quick fix that just de-registers the registry before unit tests and then re-registers it after the unit test is finished.
I included the test only which should fail spectacularly.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | psr0_in_unittests-1532056-7.patch | 2.37 KB | neclimdul |
| #5 | psr0_in_unittests-1532056-5.patch | 2.28 KB | neclimdul |
| #5 | psr0_in_unittests_test_only-1532056-5.patch | 1.19 KB | neclimdul |
| psr0_in_unittests_test_only.patch | 1.1 KB | neclimdul | |
| psr0_in_unittests.patch | 2.19 KB | neclimdul |
Comments
Comment #1
neclimdulI forgot to mention but as noted in the comment the test /can/ have false positives if a webtest where to run before it that autoloaded the Symfony/Component/HttpKernel/Bundle class but that seemed highly unlikely and we don't have PSR-0 for tests functionality so I chose something already in our PSR-0 structure very unlikely to be used.
Comment #3
Crell commentedI fully support the first patch, that does the disable/enable dance. The alternative is to do that in each unit test manually, which is nonsense.
Now, why it's failing I don't know, but I support the concept. :-)
Comment #4
neclimdulIts failing for 2 reasons. 1) I fat fingered the class name. Its Bundle\Bundle() not Bundle(). 2) Bundle won't work because it has missing namespaces. Going to have to make a class specifically for this or something.
Comment #5
neclimdulThe missing dependency was the DI container which is in core now so fixed the class name. Turns out that particular PSR-0 problem is fixed but the registry still throws an exception if PSR-0 can't load the class so I've added a test for that as well.
Comment #7
neclimdulI started to convert this to the PSR-0 test case but can't run the tests because of #1563620: All unit tests blow up with a fatal error
Comment #8
neclimdulNot that it matters for testbot but this should be commit-table...
Comment #9
sunThis patch is blocked on the following two critical bugs:
#1563620: All unit tests blow up with a fatal error
#1565718: Script test runner does not clean after fatal error
Comment #10
neclimdulAnother note, this is moot if #1541674: Remove the registry gets committed which is the obvious end goal.
Comment #11
sunI'd be fine with committing this, but yeah, I also agree this is obsolete once #1541674: Remove the registry lands — there only seems one remaining blocker for that to happen.
@neclimdul: So, leaving for you to decide. :) If this blocks you from writing good unit tests right now, then I guess it makes sense to commit it.
In any case, this patch needs to be re-rolled to move the new Registry test case into the new PSR-0 structure for tests.
Comment #12
sunoh, and... would it make sense to try to backport it, in any case?
That said, I'm not really interested in working/improving tests for D7... but technically, this seems to be a valid fix for D7...
Comment #13
neclimdulWell a blocker is Views converting to PSR-0 which is likely non-trivial. I don't feel strongly this /needs/ to be committed because I use this code myself in the plugin patch as a seperate base unit test for my plugin tests. Anyone needing it could easily do something similar and just follow up by removing it. Or we can commit this now and they can just use the normal base and we do the setUp conversion for them. Its just a nicety for people actually using unit tests(which should be everyone but...)
I'll try and reroll this today. Should be trivial enough and then we can let the committers decide.
Comment #14
sunSure, as I mentioned, I'm totally fine with that. Make it go straight to RTBC, please :)
Comment #15
sun#1541674: Remove the registry landed, so this is obsolete.