Problem/Motivation
The TestDiscovery
cache leads to DX issues while trying to write tests.
Write a test or two, look for them in the Simpletest UI, and they won't be discovered. That's because the cache took over.
Using run-tests.sh
to run tests turns off the caching behavior, so new tests will be discovered there.
Thus the only use-case for the cache is to speed up the display of the Simpletest UI. The Simpletest UI might end up being removed for Drupal 9, and we are trying to deprecate simpletest module: #2866082: [Plan] Roadmap for Simpletest
See #17 for some timing information. The majority of the time spent TTFB for the simpletest UI form is rendering, not test file discovery. The cache has a negligible effect on speed improvement.
This issue could be considered as part of the process of deprecating simpletest. See this related issue: #2863055: Move TestDiscovery out of simpletest module, minimize dependencies
Proposed resolution
Remove the caching behavior from TestDiscovery
.
Remaining tasks
User interface changes
API changes
Data model changes
Original issue
This always bugged me. No reason to cache the information of available tests.
The caching only hinders development.
Related issues:
#1541674: Remove the registry
#1683884: Update, simplify, and speedup run-tests.sh for PSR-0, and allow to run all tests of a module
Comment | File | Size | Author |
---|---|---|---|
#17 | 1667822_17_stupid_cache_invalidation.patch | 866 bytes | Mile23 |
#17 | 1667822_do_not_test.patch | 1.71 KB | Mile23 |
#13 | 1667822_13.patch | 2.35 KB | Mile23 |
#4 | drupal8.simpletest-cache.4.patch | 7.92 KB | sun |
drupal8.simpletest-cache.0.txt | 2.03 KB | sun |
Comments
Comment #1
BerdirI guess that's a left-over of the times when Simplenews had to load and parse all .test files which was possibly very slow.
The thing is, we're not going back to parsing PHP files, but we are going to back to recursive directory scans of possibly hundreds (a real site with contrib modules) of directories. I'm not sure I agree with removing that cache without a performance test.
Comment #2
sunWhat annoys me with that caching is that the only time I'm (reasonably) running tests is when I'm developing tests, and doing so most often means to create new test classes. Consequently, caches first need to be flushed every single time.
IMHO, Simpletest module should be optimized for developer experience. And with that, I mean the experience for active contributors, not passive developers.
But if performance tests are needed, one could do that with a D7 site, commenting out the cache_get() line. The only difference between D8 and D7 is the class autoloader. We are not manually parsing .test files in D7.
Comment #3
sunSee also: #1683884: Update, simplify, and speedup run-tests.sh for PSR-0, and allow to run all tests of a module
Comment #3.0
sunUpdated issue summary.
Comment #4
sunI could very well imagine that the discovery + info retrieval process could be made faster, too. (no changes in this patch though)
For example, via #697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit)
Comment #5
sunComment #6
sunComment #7
sunRestarted from scratch + merged into #697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit)
Not marking this as a duplicate of that issue just yet though.
Comment #13
Mile23I spent some time verifying the UI display of skipped tests for #2296615: Accurately support multiple @groups per test class. The workflow was: Show the list of tests, clear cache, reload, clear cache, reload...
By far the slowest part of the simpletest UI form is the rendering, not the file scan.
run-tests.sh uses the same test discovery code, but it only performs the scan once, and really shouldn't be using a cache.
This patch just removes references to the cache from TestDiscovery.
Still to do:
Comment #14
Mile23Comment #15
BerdirYes, I also noticed the fact that rendering that table takes far more time than discovery.
Do you have any specific numbers on how long discovery vs rendering takes?
We could relatively easily add render caching for the table (we could even make it so that we use a pre_render callback and only call the discovery on a cache miss) but we have no reliable way for invalidation, maybe we could just keep the button and that invalidates a custom cache tag that we use on that table? The result is that we only have caching in the UI and caching that actually helps a lot more than what we have right now.
Comment #17
Mile23Web Profiler reports that
admin/config/development/testing
takes about 25 seconds TTFB.Added is a patch that runs
TestDiscovery
a bunch of times to compare cached and uncached performance.Setting
$iterations
to 1, the as-is cachedgetTestClasses()
takes about 2.2 seconds. Uncached is about 1.8, which seems like a lot of overhead for managing a cache, so maybe my numbers are bad. Obviously this will vary with machine and OS and all kinds of other mysterious factors.Anyway, if you advance it to 2 iterations it's obvious that you want the cache. :-) Two cached calls takes 2.2 seconds, uncached is 3.5.
So if we're never calling it more than once we want to kill the cache, but if we need to call it multiple times per request, then we like the cache.
However, we don't like the cache due to DX and UX, because as mentioned the cache persists too well and we have to figure out that we need to clear it, and then clear it in order to recognize a new test.
So we have two options:
TestDiscovery->getTestClasses()
more than once or else live with the consequences.SimpletestTestForm
a way to discover tests without the cache. The patch below does this by invalidating the cache before TestDiscovery can use it.Also:
TestDiscovery::getTestClasses()
instantiates aSimpleAnnotationReader
but then never uses it.Comment #18
Mile23Updating IS, linking to #2863055: Move TestDiscovery out of simpletest module, minimize dependencies
Comment #19
dawehnerDo we have a follow up with the work @Berdir suggested which is caching the actual HTML of that page?
Comment #20
Mile23Behold: #2893117: Improve HTML caching of Simpletest UI test form
Comment #22
Mile23Soft-blocker for this issue could use some attention: #2893117: Improve HTML caching of Simpletest UI test form
Comment #24
Mile23The solution in #2893117-21: Improve HTML caching of Simpletest UI test form removes the cache from
TestDiscovery
, so this issue might be redundant.Comment #25
Mile23#2893117: Improve HTML caching of Simpletest UI test form is in, and it removes persistent caching of discovered tests. It keeps an internal cache of discovered tests per instance, to avoid repeated file system scans per request.
That makes this issue out of date.
Comment #26
Mile23This is re-opened because #2893117: Improve HTML caching of Simpletest UI test form was reverted, but postponed because that issue is really the solution.
Comment #27
Mile23And now we have #2893117: Improve HTML caching of Simpletest UI test form back again, which still makes this issue outdated by removing the caching of test classes from TestDiscovery, other than a per-request cache.