Problem
-
#2257769: Adding an Entity Reference field in the Field UI throws a PHP notice; fails to add field added this PHPUnit test to Path module:
namespace Drupal\path\Tests\Field; use Drupal\Tests\Core\Field\FieldDefinitionTestBase; /** * Tests a field definition for a 'path' field. */ class PathFieldDefinitionTest extends FieldDefinitionTestBase {
This is where it gets really really confusing:
Drupal\path
is Path module's regular runtime namespace;\Tests
for Simpletest are loaded through this.Drupal\path\Tests
is Path module's PHPUnit test namespace; its PHPUnit tests are loaded through this.- Test classes in Path module are completely intermixed; PHPUnit tests live right next to Simpletest tests.
FieldDefinitionTestBase
is not a Simpletest base class, it's a PHPUnit test base class.
Root cause
A single namespace is mapped to two directories.
Namespace Class Location
------------------ ---------------- ------------------------
Drupal\foo\ Tests\FooTest ./src/Tests/FooTest
Drupal\foo\Tests\ FooTest ./tests/src/FooTest
↓
Classname Location
----------------------------------- ------------------------
Drupal\foo\Tests\FooTest ???
Scalability
In the abstract, we're facing different "categories" of tests; or in other words, different test suites.
- #2304461: KernelTestBaseTNG™ introduces a second "category" of PHPUnit tests, which should be executed separately from unit tests.
- PHPUnit has a built-in test suite facility, which allows to discover and execute tests of a certain category only.
- Test suites are driven by configuring different filesystem discoveries; i.e., the test class files need to be located in separate directories.
- By moving unit tests into a
./Unit/
subdirectory, other subdirectories can be used for different test suites. - The first other test suite will be
./Kernel/
. There's active work in progress to introduce./Web/
(based on PHPUnit). - Since this issue needs to change the namespaces of test classes anyway, it would be wise to directly include the move into a subdirectory.
Proposed solution
Type Classname Location ----------------- ------------------------------ ---------------------------- Simpletest (Web) Drupal\foo\Tests\*Test ./src/Tests/*Test.php PHPUnit Unit Drupal\Tests\foo\Unit\*Test ./tests/src/Unit/*Test.php PHPUnit Kernel Drupal\Tests\foo\Kernel\*Test ./tests/src/Kernel/*Test.php
In other words:
Type Base class Location ----------------- ------------------------------ ---------------------------- Simpletest (Web) Drupal\simpletest\WebTestBase ./src/Tests/*Test.php PHPUnit Unit Drupal\Tests\UnitTestCase ./tests/src/Unit/*Test.php PHPUnit Kernel Drupal\Tests\KernelTestBase ./tests/src/Kernel/*Test.php
As mentioned above, web tests will soon transition from Simpletest to a new PHPUnit test suite, too. Likewise, Behat/etc testing presents yet another discrete test suite. The proposed organization of test suites in subdirectories allows us to introduce support for new test suites at any point in time later on.
This issue will only move the unit tests though.
.
.
.
Proposed solution (original/obsolete)
The ambiguity/clash of \Tests
is implicitly eliminated and obsolete when PHPUnit tests are moved into Drupal\Tests\foo
:
Classname Location
----------------------------------- ------------------------
Drupal\foo\Tests\FooTest ./src/Tests/FooTest
Drupal\Tests\foo\FooTest ./tests/src/FooTest
Comment | File | Size | Author |
---|---|---|---|
#55 | test.phpunit-ns.55.patch | 152.5 KB | sun |
#55 | test.phpunit-ns.preparation.do-not-test.patch | 10.06 KB | sun |
#53 | test.phpunit-ns.53.patch | 152.24 KB | sun |
#53 | test.phpunit-ns.preparation.do-not-test.patch | 9.8 KB | sun |
#50 | test.phpunit-ns.50.patch | 151.41 KB | sun |
Comments
Comment #1
sunComment #2
sunComment #3
sunInterestingly, PHPUnit tests for Drupal core are using
But yet, PHPUnit tests for modules do not:
Comment #4
BerdirYeah, that's how it is :)
TestBase is not a new pattern, see \Drupal\Tests\Component\PhpStorage\PhpStorageTestBase.
UnitTestCase is really the only exception and once UnitTestBase is gone, it would be nice if it wouldn't be, but we can't rename everything ;)
Comment #5
tstoecklerYes, #3 trips me up every time. Every time I write a unit test I have to look into like 10 different test classes just to ensure to myself that I'm doing it "right". We should really fix that!
Comment #6
sunFixing #3 might actually be enough to resolve this confusion altogether. It would set a clear differentiation and standard:
Comment #7
donquixote CreditAttribution: donquixote commentedAre you sure?
Since the PSR-4 move, tests/src in each module is wired to Drupal\foo\Tests, not Drupal\Tests\foo.
I asked several times in #2083547: PSR-4: Putting it all together if this is going to be ok. No opposition there.
Although sure we can still move this if we need to.
I personally think ownership (Drupal\foo) is more important than classification (test or not test). So imo, Drupal\foo should come first, and then Drupal\foo\Tests or Drupal\foo\PHPUnit, etc.
We also have another mess, that is, fixtures and example modules for both phpunit and simpletest all together in the tests folder.
Maybe a solution could be to always name PHPUnit tests as *TestCase, And Abstract*TestCase, whereas simpletest tests would be *Test and *TestBase?
Or to really clean it up, we should have $module_dir/phpunit and $module_dir/simpletest..
How long is this going to last? Will we ever fade out simpletest?
Btw, see also #2273311: FieldDefinitionTestBase::getNamespacePath() breaks with PSR-4
Comment #8
sunI intensively worked on test classes, test discovery, and test execution over the past weeks. More than a hundred of times I got tripped up by phpunit test class names/namespaces. I.e.:
First thing you do: Check the namespace. It's
foo\Tests
. So you check./src/Tests/
. 404.Meh. Check
./tests/src/
instead.Misery. :-(
--
Aside from a massive patch to perform an in-place search/replace, are there any objections to changing the namespace of phpunit tests of modules to follow the clean separation of phpunit tests of Drupal core?
Comment #9
donquixote CreditAttribution: donquixote commentedIn addition to what I already said in #7, here is a look at Symfony:
https://github.com/symfony/symfony/tree/master/src/Symfony/Component/Con...
namespace Symfony\Component\Config\Tests
So, Symfony puts ownership before purpose.
Ownership = Symfony\Component\Config\.
Purpose = Tests\.
We should look at more examples (preferably stuff that has bundles or sub-packages). But personally I would also prefer ownership first.
I agree with the problem though, and maybe we can find a different solution to that..?
E.g. 'src/Simpletest/' => 'Drupal\foo\Simpletest\\' ?
One obvious problem is the confusion this would bring to D7.
Otherwise, see #7.
Comment #10
sunNot sure why you bring up the topic of ownership. That's irrelevant from my perspective, because both
Drupal\foo\Tests
andDrupal\Tests\foo
are owned byfoo
.The root cause is a name clash in namespaces. A single namespace is mapped to two directories.
The ambiguity/clash of
\Tests
is implicitly eliminated and obsolete when PHPUnit tests are moved intoDrupal\Tests\foo
:Due to that I don't see a reason for renaming the
\Tests
subnamespace of Simpletest tests. Instead, the opposite; the parity/consistency of\Tests
is a good DX.Comment #11
sunRemoving the
TestBase
vs.TestCase
topic from this issue title + summary to ensure focus. Might be worth to discuss/resolve in a separate issue though.Comment #12
sunComment #13
ParisLiakos CreditAttribution: ParisLiakos commentedYes, i have been tricked by this many times, and messed up namespaces, with patches that went in and had to open followups to fix.
This needs to be fixed somehow, for sure, we would be doing ourselves a favor.
#9: the ownership first pattern, doesn't apply already for Core and Component namespaces
Drupal\Tests\Component
Drupal\Tests\Core
I do not like it, but thats how it is.
Based on the above #6 would make sense and it would be consistent with that
Comment #14
sunUnless I'm terribly mistaken, the current test discovery + runner code in HEAD doesn't even need any additional adjustments, since the namespace is ignored anyway right now (soon to be changed by #697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit)).
Comment #16
sunSorry, of course, autoloading of dependencies/ancestor classes fails without adjusting the classloader namespaces.
Additionally, there are a few false-positives — which further underline the importance of this patch.
Attached interdiff needs to be applied on top of the shell commands in #14, in case this patch needs to be re-rolled.
Comment #17
xjmSo:
Does this really solve the problem? Yes, at least the namespace/directory mapping is less confusing. But I still think I might be confused as to whether I should use
foo\Tests
orTests\foo
. And it also seems to make it so that all PHPUnit tests are the property of Drupal rather than the module.Why don't we just have actually specific namespaces for unit tests versus simpletests rather than swapping around the
Test
magically? Like.Drupal\mymodule\PHPUnitTests
orDrupal\mymodule\SimpleTests
. Or something.Comment #18
xjmI believe the implications of this change are that tests not updated properly silently don't run, so it makes sense as a beta deadline.
Comment #19
sunPutting your PHPUnit tests outside of your regular runtime namespace is a common practice for PHP projects. The generic pattern is:
"Drupal" is just the shared vendor name in our case, not the owning property.
IMO, this pattern is easy to learn and trivial to understand. It's new for your eyes right now, but once it's in HEAD, it won't take long for you to get a habit of it.
It's also consistent with PHPUnit tests of Drupal core components, which are using the separate
Drupal\Tests\
namespace already:→ Parity + consistency.
I'm -1 on the idea of using subdirectories within \Tests, because:
Comment #22
sunDraft change notice: https://www.drupal.org/node/2301057
This patch may need a disruption window (in case we still have that concept today) — 99% of the patch is scripted, so I can re-roll anytime prior to commit intentions.
Comment #23
sunSince the patch is mostly scripted, it does not make sense to (re-)roll intermediate patches, as they will fail to apply shortly after.
This issue should be marked RTBC if/when there is an agreement on the/a proposed solution, in which case a new patch will be rolled against HEAD.
Comment #24
sunRe-rolled against HEAD.
Now using a patch-serial to perform the preparation in a discrete commit upfront, and bulk of test changes second.
Comment #25
sunBetter issue title, hopefully.
Comment #28
sunComment #29
sunWork on #2304461: KernelTestBaseTNG™ revealed that we need a way to differentiate between "types" of test class files by their filesystem locations only.
That is, because PHPUnit's configuration (in particular multiple test suites) are driven by a rather simple recursive directory scan. The scan picks up every class that implements the
PHPUnit_Framework_Test
interface (e.g., classes inheriting fromPHPUnit_Framework_TestCase
). Therefore, in order to enable multiple test suites, you need to scan the right directories.In PHPUnit, a common configuration pattern is to split your tests into "small", "medium", and "large" tests (whereas these labels are meant in an abstract way). The "small" test suite means pure unit tests only - no process isolation, and the entire test suite is supposed to complete in ~1 second. The new Kernel test suite maps to a typical "large" suite; every test takes ~1 second and the whole suite takes ~15 minutes.
The primary reason for splitting tests into suites is to run selected suites at the right time, when sensible; i.e., just the super-quick small suite when you're hacking, optionally leaving the larger suites to CI/QA/build tools.
Due to that, I'd like to take back my earlier reservation against using different subdirectories and suggest the following:
In other words:
The subdirectory name pattern is extensible. So in case we find a way to also run web/acceptance tests via PHPUnit, then those tests would go into a new "Web" subdirectory. Ditto for any other possible new test suite / types of tests for which we might have a use-case in the future.
This essentially means to move a lot of tests, and tests in the previous
./tests/src
directory (sans subdirectory) will no longer be discovered.This particular issue here would only move unit tests into the
Unit
subdirectory (and change namespaces accordingly, as in existing patches).Thoughts?
Comment #30
Mile23SimpleTest and PHPUnit can both go into
tests/
, and it won't matter that they share directories, because you'll look at the superclass and the @group tag to find out where you are.@group Unit
and@group Kernel
separates the different types of tests. Each suite can specify the group it wants to run, so you'd dophpunit --testsuite Kernel
SimpleTest infrastructure can be simplified to just autoload all the classes from
tests/
, and not ever have to de-autoload or whatever it does currently to avoid loading those classes at runtime.Also, it looks like SimpleTest is on the way out, so who cares what it thinks? :-)
Comment #31
sun@Mile23: It looks like you are mistaking some things.
--group
is an optional mechanism to filter all available tests of the to be executed test suites by@group
names. Tests will not have@group Unit
annotations. That would be the utterly wrong tool for the job, completely unmaintainable, and a total DX pain.@group
annotations are only used to further categorize tests within a test suite, so as to allow to execute a subset of tests. If the--group
option is omitted, then no filtering happens and all tests in the to be executed test suites are ran.--testsuite
is a mechanism to specify the test suite to execute. Each test suite defines the test files to discover. PHPUnit will execute every discovered test class that derives fromPHPUnit_Framework_TestCase
, which factually means every test file. Test suites are used to establish independent sets of tests. A test suite is defined by custom criteria; typically the size/duration of contained tests plays a key role for defining test suites.Group filtering is applied after the fact, after all tests have been discovered and loaded. A test suite filters tests upfront, by discovering test files from selected directories only.
Comment #32
sunWriting a script to perform the necessary changes won't be trivial, so it would be great to gather consensus on a proposed solution as soon as possible.
Updated the issue summary.
Comment #33
ParisLiakos CreditAttribution: ParisLiakos commentedjust want to give my +1 for the proposed resolution.
biggest win:
Comment #34
sunAttached patch performs the proposed changes. As before, I separated the preparation into a first commit; the second is fully scripted.
Comment #36
sunThe conversion script did not account for namespace declarations using the bracket syntax.
Comment #39
sunWeird. That testbot failure doesn't make any sense. Seemingly loads a PHP file that is clearly renamed by this patch.
In trying to debug that, I found one phpunit test that is wrongly located in the Simpletest tests directory (most likely caused by the confusion that this issue is about to fix). Now fixed in the first commit. However, appears to be unrelated to the testbot failure.
Wondering whether it's caused by the patch-serial. Therefore attaching two patches this time.
Comment #41
sunOK, the regular patch passed. Looks like the testbot has problems with patch-serials. I'll attach two patches from now on (so as to keep the necessary non-scripted changes reviewable).
Comment #42
sunRe-rolled against HEAD.
Comment #45
sunRe-rolled against HEAD.
Comment #47
tstoecklerI very much like the idea of grouping tests by test suite.
Like @Mile23 I wonder, though, if there's any technical reason for not moving the SimpleTest-based Tests into the
tests
directory as well. They could be put into thetests/src/Web
directory to denote a (pseudo-)Web test suite. The downside would be the confusion of having both PHPUnit tests and SimpleTest tests right next to each other, but for me personally the consistency of having everything related to tests in a single directory wins over that. Alternatively, we could group tests by "test type", i.e. put PHPUnit tests intests/phpunit
and SimpleTest tests intests/simpletest
. With Behat more and more becoming a thing in Drupal 8 that would perhaps pave the way fortests/behat
as well.Thoughts?
Comment #48
sunre: #47: Simpletest/Web tests are not touched here, because there's active work in progress to execute web tests via PHPUnit — i.e., instead of mixing legacy tests into the new structure, we only account for further test suites here; the plan is to properly convert them and only at that point we move them into
./tests/src/Web/
.Comment #49
tstoecklerSure, makes sense.
Regarding the actual patch, I am wondering why you are replacing
__DIR__
withDRUPAL_ROOT
in a few places. Not only does this seem unrelated, but also the former is generally preferred, as it does not constitute a global dependency. Can you elaborate?Edit: Ahh, because the files are being moved then in the second commit. Still, is there a strong reason for not simply adapting the
__DIR__
lines?Comment #50
sunYes. However, I'd like to avoid a discussion on excessive
dirname(__DIR__)
recursions vs.DRUPAL_ROOT
in tests on this issue, so attached patch addresses #49.Comment #52
sunBlocked on #1966436-243: Default *content* entity languages are not set for entities created with the API to move a misplaced kernel test. Will re-roll directly afterwards.
Comment #53
sunThat's fixed. Re-rolled.
Comment #54
XanoComing from a single testing framework, we have come to use two frameworks right now, but that number will grow in the future. Behat is going to be part of core, and we may well leverage more frameworks than these three long before we release Drupal 9. I'd rather separate tests by framework than put them too close together.
There is another
@see
to this issue further down in the file.There is another @see about this issue further down in the same file.
Comment #55
sunI interpret that as a +1 to the proposed organization (and equally a +1 for not mixing legacy Simpletest tests into
./tests/src/*
).And yes, the proposed filesystem organization scales in any way we like to. Just to provide some more concrete examples:
./tests/src/Xyz/*
subdirectories../tests/features/*.yml
files…?./tests/qunit/*.js
Fixed that stray @see.
Comment #56
donquixote CreditAttribution: donquixote commented+1 from me for separate PHPUnit test suites, and using subdirectories and sub-namespaces to organize them, and also to organize non-PHPUnit tests.
I have not spent much thought about whether
Kernel\
is a good name for this, but I have no strong reservations either.I am still not convinced about whether
Drupal\Tests\foo\
orDrupal\foo\Tests
is preferable, but I leave that to others.And I am not convinced about using this distinction to differentiate simpletest tests from PHPUnit tests. Would make more sense to me to use named sub-namespaces for everything. E.g.
Drupal\foo\Tests\Simpletest\
orDrupal\Tests\foo\Simpletest\
. But if might be ok to leave that as it is for now, if only for the sake of reducing the patch size.Maybe we want a follow-up to move Simpletest tests into
tests/src/Simpletest/
.Another question is whether we want subdirectories for fixtures / test resources as well.
Atm, modules for simpletest live in the tests/ folder, outside tests/src, together with fixtures for PHPUnit. This can be confusing, but on the other hand, it allows shared resources for multiple test suites.
Comment #57
tstoecklerI agree 100% with #56 but I think #48 adresses it sufficiently: That is we are leaving SimpleTests alone in this issue in order to not vastly expand scope. Furthermore, instead of simply moving them straight over in a follow-up we can hopefully convert them to Behat along the way.
Reviewed the -do-not-test patch again and skimmed over the actual patch to verify there's nothing crazy going on.
This has been sitting for a while and so far there has been no real objection. Let's clean this mess up! RTBC.
Comment #58
Dries CreditAttribution: Dries commentedIt is not 100% clear what major problem this solves but this patch also seems pretty harmless. I decided to commit it because it appears to unblock other work on test improvements.
Comment #60
tim.plunkettAs far as I can see, this completely prevents tests from being run via simpletest.
https://github.com/drupal/drupal/blob/8.0.x/core/modules/simpletest/src/...
TestDiscovery::findAllClassFiles() explicitly looks for
"Drupal\\$extension\\"
, which now does not match any of these.Should we revert this, or does someone have a quick fix?
Comment #61
tim.plunkettHmm, it seems PIFR/PIFT is working around this, opened #2333747: run-tests.sh ignores phpunit tests when using the --module flag to fix this without reverting.
Comment #62
xjmhttps://www.drupal.org/node/2012184 needs to be updated for this change.