Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This is a pretty handy test for catching coding errors before we commit them but it takes 2-3 seconds to run on my quick little SSD. The attached patch achieves the same test in ~209ms. It does loose the line number of the failure but its easy enough to search a file for the Drupal\Core.
I'm generally in the camp of not focusing on optimizing tests but this is a really slow unit test and this really achieves the same thing.
Beta things, this is testing and changes no api's or add features or any of that so its clear.
Beta phase evaluation
Issue category | Task because things work, this issue only speeds up execution. |
---|---|
Issue priority | Normal because nothing is really broken, this is an optimization. |
Unfrozen changes | Unfrozen because it improves automated tests. |
Comment | File | Size | Author |
---|---|---|---|
#11 | optimize-2432939-11.patch | 2.9 KB | neclimdul |
#11 | 2432939-11.interdiff.txt | 677 bytes | neclimdul |
#6 | interdiff.txt | 2.24 KB | Mile23 |
#6 | 2432939_6.patch | 3.11 KB | Mile23 |
Comments
Comment #1
neclimdulto the testbot
Comment #2
franksj CreditAttribution: franksj commentedBefore patch:
After patch:
Vast improvement in speed.
Added
use Drupal\Core\Access;
to core/lib/Drupal/Component/Annotation/Plugin.php and the test fails as expected.Looks good!
Comment #3
Mile23Added beta evaluation.
Comment #4
Mile23Comment #5
alexpott@neclimdul thank you - this has been bothering for a while. I think we should add a quick test for
assertNoCoreUsage
to this class. It should be simple enough by catching the php unit fail exception. See https://stackoverflow.com/questions/3917909/how-to-indicate-that-a-phpun...Comment #6
Mile23Added a test using vfsStream.
The test is on the same test class as the method being tested, mostly because assertNoCoreUsage() is protected and I'm trying to catch a plane. :-) Easy enough to move to another class and mock up if it's too weird.
Comment #7
neclimdulworks for me.
Comment #8
alexpottI think this can be simplified to something like:
Comment #9
neclimdulAs suggested, seems to work. Swapped one of the values in the provider to confirm it failed as expected.
Comment #10
Mile23With patch:
Time: 6.86 seconds, Memory: 120.75Mb
Without patch:
Time: 12.88 seconds, Memory: 121.75Mb
I bet you $10 the default message is more informative than an empty string.
Comment #11
neclimdulDope, I started to write this, got pulled away and sat down and said "what was I doing? egh, it works lets post it."
Comment #12
neclimdulNote: with the empty string you still get the default message. The messages are additional in phpunit not replacements like in simpletest.
Comment #13
Mile23Ossum. :-)
I applied the patch in #11. It still speeds everything up. It caught use statements in Component that used Core namespaces which I added to random component code. It also didn't false-error on @see for the same namespaces.
It has tests with super-duper output messages.
I'm calling it RTBC, even though I wrote some of it. Feel free to chastise me for this.
Comment #14
alexpottCommitted a7ab29e and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.