Follow-up to #697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit) + #2293825: Various test classes do not have a "Test" suffix
Objective
- ~10% of all class files that are evaluated by the test discovery are not tests (with just core).
- Every class is introspected + loaded persistently into memory, which increases memory/resource consumption.
- Limiting test discovery to
*Test.php
filenames would avoid needless introspection/reflection of ~130+ classes.
Tasks
- Change the file filter in TestDiscovery.
- Profile/benchmark the performance difference.
- Discuss, based on hard numbers.
Notes
PHPUnit only discovers*Test.php
files by default, so as to ignore test base classes + any other possibly existing fixtures.Following PHPUnit's (industry standard) lead would definitely improve consistency across testing frameworks. → Learn once, stay familiar.- Drupal implements test suites which can load different test types under the phpunit tool. These suites inherit their file scanning abilities from
TestDiscovery::scanDirectory()
. So any improvement toscanDirectory()
will be an improvement for our implementation of PHPUnit as well.
Counter-Arguments / Concerns
-
The filter is applied during discovery already. A wrongly named test file is not executed and does not throw an error, because it is not found in the first place.
This could result in "false-positive" test results, because a newly added test was not actually executed.
Counter-Counter-Arguments:
*Test.php
filename suffixes are industry standard. The class is a test, so you call it a*Test
. PHP developers are used to this notion already. Even Drupal core follows suit since D8 (and similarly in D7 already, just with a*TestCase
suffix).- Tests are exclusively authored by PHP developers. Catching mistakes like a malformed test class file/name is the job of peer-reviews.
- A git repository pre-commit or push hook would be able to catch such mistakes and prevent them from entering the mainline. That is a much more appropriate location and point in time for performing sanity checks.
Comment | File | Size | Author |
---|---|---|---|
#61 | 2296635-61.patch | 2.73 KB | Hardik_Patel_12 |
#51 | 2296635-51.patch | 4.41 KB | dawehner |
#49 | 2296635-49.patch | 899 bytes | dawehner |
#47 | 2296635-47.patch | 846 bytes | dawehner |
#36 | interdiff-33-36.txt | 1.46 KB | Anonymous (not verified) |
Comments
Comment #1
sunComment #2
chx CreditAttribution: chx commentedFraming this as performance is just the kind of insidiousness I expect. It's not about performance. It's about piss poor DX. It's not like the parent found 20 tests scattered among all of core that wouldn't be picked up. My suggestion is: namespace + annotation. Learn plugins once, reuse for tests.
Performance is a noble goal except when you are so fast you don't find the very thing you want to run.
Comment #3
sunThis issue is about test discovery performance. Let's refrain from making arbitrary claims without data points. I did phrase the issue summary accordingly already.
Comment #4
chx CreditAttribution: chx commentedComment #5
sunSorry, but that's not the goal nor the intention of this issue. (the suggested change is the opposite of this issue)
FWIW, this issue is the result of excessive performance profiling on which I worked almost exclusively over the past weeks, which gave sufficient hints that skipping the needless processing of (hundreds of) non-test classes most likely makes a measurable and visible difference.
However, I did not benchmark/profile the situation of skipping non-test class files yet, so there are no concrete data points to argue for or against yet. Not planning to do that before the parent issue has landed, so still postponed.
Comment #6
tim.plunkettComment #7
chx CreditAttribution: chx commentedRestoring the IS since my proposal is in a different issue already.
Comment #8
Mile23This seems kind of settled...
Comment #9
Mile23Hmm. Found a todo about this.
Comment #11
dawehnerThe developer experience problem can be solved using the following patch:
It checks for every executed test, whether the filename ends with
Test.php
, otherwise throws an error.Comment #13
dawehnerOne way of fixing is to use case insensitive regex checking.
Comment #15
Mile23The @todo is in the docblock for
Drupal\simpletest\TestDiscovery::scanDirectory()
.The patch in #13 deals with PHPUnit tests only and is probably out of scope here. Tho make another issue because it's a good idea. That said, PHPUnit tests now use
scanDirectory()
to find test suites.Comment #16
Mile23Comment #17
dawehnerSure, here we go: #2793701: Ensure that phpunit tests end with Test.php
Comment #21
Mile23#2949363: Impossible to make trigger_error in some files without test fails adds an interesting wrinkle because our test discovery system ends up loading discovered files. If a test trait has a @trigger_error(), it will trigger during test file scans, rather than test run time.
If TestDiscovery did not allow files to end with anything other than *Test.php, we wouldn't load trait files for introspection, and they could be fully deprecated. The down-side is that test files with non-standard names would be excluded, so we might get a false positive based on an ill-named test. This would not be caught by our coding standards test listener because the test file would not be discovered.
Here's a patch which does the filtering on *Test.php.
Comment #22
Anonymous (not verified) CreditAttribution: Anonymous commentedLet's close #2949363: Impossible to make trigger_error in some files without test fails like duplicate. Please add credit to @timmillwood and @Lendude who helped in solving the problem.
This is blocker for #2946419: Entity: Convert system functional tests to phpunit and other Simpletest ->PHPUnit issues. So change status to Major. Perhaps it is also Bug report, not Task.
Based on @Lendude's post, lets add special test for this changes. I tried made Unit tests, but it is not works, so BTB test.
Nit question:
What about
Interdiff with #21 is test-only.patch.
Comment #24
Anonymous (not verified) CreditAttribution: Anonymous commentedMove test to Kernel + #22 nit code change.
Comment #26
longwaveI think this code in getTestClasses() can be simplified slightly now, as every class name in this loop should end in Test already?
Comment #27
dawehnerWe could document a line: Ensure that files end up with Test.php to replicate the phpunit built in behaviour.
Comment #28
KingdutchI've simplified the catch statement of getTestClasses() as requested by #26. I've adjusted the documentation to indicate the new behaviour. Group annotations, traits or test fixtures shouldn't make it to this piece of code anymore (as they'll be filtered out by the Test.php requirement). I didn't dare remove the if statement because I wasn't certain what classes would be generated for drupal_migrate (e.g. a google search helped me find a sandbox that contained a "ContentFieldTest.php" file containing the schema for a content_field_test table).
I've also added the suggestion by dawehner from #27 with regards to the comment verbatim.
Comment #29
Anonymous (not verified) CreditAttribution: Anonymous commentedLooks nice! RTBC +1
Comment #30
dawehnerIsn't this still helpful information?
Comment #31
KingdutchRe #30
At the time, I reasoned that these classes would no longer make it to this part of the codebase due to the new filename restrictions.
I'm not sure that at this time I can come to the same conclusion so maybe that was too much removal indeed :/
Comment #32
dawehner@Kingdutch
Let's add it back, as it makes for example the patch easier to review. Otherwise this looks rocksolid.
Comment #33
KingdutchAdded back the text
Comment #34
dawehnerThank you @Kingdutch
Comment #35
alexpottI don't really get why this is here. If it does need to be here the comments on it need to explain it. Given that the whole point of this change is to limit to *Test.php I'm not sure why we need this. Ah I see I guess it's TestDiscoveryTest - I'm not sure once we limit to 'Test.php' this is really necessary. And if it is then the trait needs an @see to the test and the test needs an @see to this thing. Personally I'd remove both.
Comment #36
Anonymous (not verified) CreditAttribution: Anonymous commentedThe only reason is the desire to prevent regression in the future. But maybe it's too zealous insurance. After #2946419: Entity: Convert system functional tests to phpunit we will have a real deprecated trait. And we already have 6 tests that use
getTestClasses()
method.So, let's remove both and boldly step forward :)
Comment #37
dawehnerYeah, if needed you can always add the
trigger_error
call later.Comment #38
alexpottI read this line in the issue summary
But I couldn't find any evidence that this is true - maybe I missed something. If we're not already limiting to *Test.php then I think we need to deprecate support for tests that do not end in *Test.php first and then we can remove this in Drupal 9.
Comment #39
alexpottYeah we support other endings :(
Also the argument about what PHPUnit supports is not really correct - it'll run any file you tell it to ^^
Comment #40
dawehnerDoes it run this also when we just execute the testsuite?
Comment #41
alexpottYep.
Comment #42
Mile23OK, then we still have the problem of reflection against traits that trigger deprecation errors during discovery: #2949363-23: Impossible to make trigger_error in some files without test fails
Comment #43
alexpottLet's do this in steps. I think given that tests are meant to be run we could make an argument to deprecate this in 8.6.x and remove support in 8.7.x. This would need release manager support. But at least that would give people time to fix any tests.
The problem is if we remove support we have no way to inform people because we're just no longer scanning the files.
Comment #44
Anonymous (not verified) CreditAttribution: Anonymous commentedReopen #2949363-25: Impossible to make trigger_error in some files without test fails.
Comment #46
alexpottThis is still a very good idea! Because we do this we caused https://www.drupal.org/project/drupal/issues/3026414#comment-12940455
Comment #47
dawehnerJust seeing how much would fail if we throw a deprecation.
Comment #49
dawehnerThis time it actually checks whether a valid Test has an invalid path.
Comment #51
dawehnerThis should rename the tests which are wrongly named in Drupal core.
Comment #52
Mile23"...will not be allowed in Drupal 9.0.0."
Also add $pathname to the deprecation message so there's only one @trigger_error().
This is the closest change record I could find but it doesn't say to use Test.php suffix for your file: https://www.drupal.org/node/1543796
Comment #55
Neslee Canil PintoComment #56
jungle@Neslee Canil Pinto, thanks for working on this! Would be great to attach an interdiff, see https://www.drupal.org/documentation/git/interdiff
Comment #57
Neslee Canil PintoComment #58
jungle#55 did unexpected change. And did not address #52
But #55 spotted the wrong renaming from my understanding.
Orginally, it's TestSettingSummariesContentType.php. In #51 it's renamed to TestSettingSummariesContentTypeTest.php, (Test append, but The leading Test still there). #55 renamed it to SettingSummariesContentTypeTest.php by removing the leading Test.
Instead of working upon #55, this patch started from #51. Made two changes.
Comment #59
jungleA simple Change Record added https://www.drupal.org/node/3123888
Comment #61
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedRe-rolling the patch, kindly review.
Comment #62
jungleI do not think the re-roll in #61 is unnecessary, changing back the target branch to 8.9.x
Comment #63
jungleThe target component should be PHPUnit, I guess. And #58 only deprecated/@trigger_error for simpletest tests, not all types of test files covered.