Problem/Motivations
The lib folder have no UnitTest associated with each classes components. We could make the process of learning a class responsibility easier by providing a unit-test class which demonstrates the possibilities of the production class.
Proposed resolution
Software good practices are to create a parity test/ folder which takes the same structure as the lib/ folder and test every classes like the following:
- lib/
- Drupal/
- Core/
- Config/
- FileStorage.php
- Component/
- test/
- Drupal/
- Core/
- Tests/
- Config/
- FileStorageTest.php
- Component/
Separating the production code from the testing code also helps autoloaders to perform betters and provides a better class organisation in my opinion. We could do the same for every modules.
Resources
Attached a simple PoC which refactored the FileStorageTest to use a mocked filesystem (virtual filesystem).
Comment | File | Size | Author |
---|---|---|---|
#3 | POC-FileStorageTestNG.patch | 7.18 KB | Sylvain Lecoy |
#5 | POC-FileStorageTestNG.patch | 9.74 KB | Sylvain Lecoy |
#5 | FileStorageCoverage.jpg | 125.97 KB | Sylvain Lecoy |
POC-FileStorageTestNG.patch | 5.04 KB | Sylvain Lecoy | |
Comments
Comment #1
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedComment #1.0
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedtypo.
Comment #1.1
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedtypo.
Comment #2.0
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedChanged link
Comment #3
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedHere is the progress on the class test.
Still not finished yet but interesting how the testDelete() method demonstrates how the delete() function is leaking on other sub-systems, compromizing the real isolation of this unit-test.
I have mocked the super-minimal required environment to be able to assert that a file has been deleted. See by yourself the number of services which are required.
Comment #3.0
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedtypo.
Comment #3.1
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedChanged folder hierarchie to include Tests/
Comment #5
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedHere's the "feature complete" class with the coverage report. :)
Comment #6
donquixote CreditAttribution: donquixote commentedNo strong opinion, just some quick thoughts/feelings.
My first worry is that this will further blow up our directory structure. I already find the PSR-0 deep folders unpleasant, and now we get an additional tree.. ouch.
There might be good reasons for this "common practice", so the above is just a personal feeling.
For the clear separation of directories: I would say, this is already the case even if we put tests into the lib folder. Simply because tests are all in the lib/../Tests subfolder, this already gives us some separation, which seems good enough for my taste.
For the "parallel" structure of "real" classes vs their unit tests: Sounds reasonable, but where do web tests and other tests go, that do not map 1:1 to a "real" class? I suppose, in the same directory?
Another thing: Afaik, some D7 modules use the /tests/ subdirectory for other test-related material. This could be test-related submodules, etc. If we occupy this folder as a PSR-0 root, those other things would need to move elsewhere. That's not a biggie, but something to consider.
EDIT:
To clarify, I support the idea of one test per class. I just propose to leave those files into
lib/Drupal/$module/Tests/
, and not open up a second PSR-0 root dir.Comment #7
XanoThe reason is that it's easy to find test files for classes. It's nothing more than a naming convention that adds predictability. It means that every \Drupal\foo\Bar class is accompanied by a \Drupal\Test\foo\BarTest class, no exceptions.
Adding modules in module folders IMHO makes directory structures worse, because it does not add predictability. A convention to put tests in a specific directory does not.
Comment #8
donquixote CreditAttribution: donquixote commented(topic starter)
How that?
I studied a lot of autoloaders, and wrote a bunch of them myself, and I don't see how this would make a difference.
Class loading with APC on won't change. With APC off it depends on the number of registered namespaces, and this doesn't change if you put the classes into a different directory.
It could make a change if we did build a class map based on filesystem scans. But we could also just omit the /Tests/ subdir in those scans, it would have the same effect. And besides, I suspect that those class maps are not really faster than the apc cache, or are they?
Comment #9
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedYes you are right, with APC off it depends on the numer of registered namespaces.
With APC on however, it checks the existance of a file with an "apc_fetch" as it is bellow:
But this is not an argument against as the production site may (should?) have testing module disabled.
Comment #10
donquixote CreditAttribution: donquixote commentedYes, this is how it works.
But it really does not matter whether the test classes are in
lib/Drupal/$module/Tests/
or intests/Drupal/$module/Tests/
, this was my entire point :)I would say, no matter where we put those classes, it might be a good idea to use a dedicated classloader for tests, instead of the Symfony one, and only activate it on demand, so
- The Symfony class loader does not get blown up with a bazillion of disabled modules' "\\Tests\\" namespaces.
- The APC cache does not get blown up with test classes, which we only need every once in a while.
The separate /tests/ directory would help to prevent that Symfony class loader will ever touch any of the test classes.
Is this something we care about? Probably not. Those test classes are only ever requested when running tests, and even then it doesn't make a big difference.
Conclusion, the is no relevant impact on autoload performance.
So it's a pure DX issue, and "how does the rest of the world do it".
Comment #11
XanoIf we want to explore the option of only loading namespaces that we need: is there a global flag that says the site is in test mode? We could reuse that for the class loader.
Comment #12
tstoecklerRe #11: Yup, it's drupal_valid_test_ua.
Comment #13
donquixote CreditAttribution: donquixote commentedWhat about splitting up the \Tests\ namespace even further?
This would help to keep other tests out of the parity folder.
(and note, this post says nothing about folder structure, only about namespaces)
Comment #14
XanoWell that's the thing: class names and namespaces are identical to the namespaces and names of the classes they represent, with two exceptions:
1) Class names are suffixed by "Test"
2) There is an extra "Test" namespace between the vendor and the package.
This means that \Drupal\foo\Bar\Baz will be tested by \Drupal\Test\foo\Bar\BazTest.
Comment #15
donquixote CreditAttribution: donquixote commentedSure, I get the idea :)
My proposal was simply to change the matching to
\Drupal\foo\Bar\Baz
tested by\Drupal\foo\Tests\Parity\Bar\Baz
instead of
\Drupal\foo\Bar\Baz
tested by\Drupal\foo\Tests\Bar\Baz
(Sylvain), or\Drupal\foo\Bar\Baz
tested by\Drupal\Test\foo\Bar\Baz
(Xano)So we would get a free namespace
\Drupal\foo\Tests\..
for other test classes.Comment #16
sunA separate directory structure was intensively discussed when PSR-0 Tests were originally introduced and decided against.
The remaining part duplicates #1605512: Allow stuff in core/lib/Drupal to ship with tests
Comment #17
donquixote CreditAttribution: donquixote commentedI am fine with that, no separate directory structure.
However, this still leaves the question of whether to have "parity" unit tests for each (*) production class.
(and put them all under /lib/Drupal/$module/Tests/(Parity/))
The linked issue does not mention this at all.
(*) "each" = depending how well we keep up with writing unit tests.
Comment #18
sunSure, that's just a good unit testing practice. Ideally followed, but not a hard requirement.
I don't see why that needs any discussion?
Comment #19
donquixote CreditAttribution: donquixote commentedWe should agree on a namespace convention for those parity tests. See #15.
That's all, then we can close :)
Comment #20
sunI'm not sure I understand the question.
All test classes are in
Drupal\Foo\Tests
All non-test classes are in
Drupal\Foo
and not inDrupal\Foo\Tests
For parity unit test classes, that simply means:
All of this is a "MAY" in spec-talk. A specific layout CANNOT be enforced on all components/tests.
The actual layout for tests can vary by component and often has to. All of the following examples are perfectly fine, valid, and plausible, too:
Comment #21
donquixote CreditAttribution: donquixote commentedAs I understand, one of the ideas of the issue is to choose one of these variations as a convention / preferred way to do it.
Sure, we can also choose to leave it without a convention, if everyone is happy with that. I don't care that much, personally.
Comment #22
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedRe #20, so you mean as a general rule the 'Tests' fragment has to suffix the component namespace ?
I'll be able to update Unit Test Organization accordingly.
Comment #23
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedRe #12: this test hock should also disappear when PHPUnit will be used.
I written a page to avoid using such a test: http://drupal.org/node/1814344#test-hock.
Comment #24
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedIn #1400748: Proposal for unified namespace organization Crell said the following about a tests/ directory:
Comment #25
sunre: #21: Yes. I do not think that it makes sense to enforce or recommend any particular layout at this time. We're still lacking unit tests to begin with. So that's a discussion to have in ~2016.
re: #23:
drupal_valid_test_ua()
is only used by the base system, in order to re-route web/integration test HTTP requests to the testing site (using a different database prefix and filesystem directories). This will still be required in order to enable web/integration tests, even with PHPUnit.re: #24: That issue is very old and contained the initial proposal and discussion only. During the actual implementation of PSR-0 tests, the /lib vs. /tests separation was ditched altogether. One of the main reasons is that tests still need to access regular runtime classes of other modules (which in turn may depend on other modules), which in turn would have meant to register all regular namespaces of all modules a second time, just with a different namespace root. In short, it did not make sense. As a side-note, Symfony components contain their respective tests in a relative ./Tests subnamespace, too; i.e., in the same way/layout we're using today.
Comment #26
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedRe #25, I think it was a good intuition about the tests/ directory, no matter how old the issue is or discussions that have been performed since. Moreover, in a testing context, what is wrong registering all namespaces a second time ? This is not a performance critical environment.
In a more socially context, I see it as a good incubator for placing PHPUnit tests, separating new tests from old tests; we could tell the developers that their new tests can now go in this directory, because it will be automatically scanned by PHP Unit framework. This separation will help reducing confusion when working on a test suit which mix Simpletest and PHPUnit based tests. It will reduce confusion because the tests will be physically separated.
Comment #26.0
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedChanged libs to lib and tests to test.
Comment #36
quietone CreditAttribution: quietone at PreviousNext commentedMoving to testing component.