Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Follow-up from #1411114: drupal_valid_test_ua() is not statically cached on non-positive match, this may not be worth caching.
Comment | File | Size | Author |
---|---|---|---|
#13 | 1436684_failing_tests_d7.patch | 957 bytes | Cyberwolf |
#11 | drupal8.test-ua.11.patch | 8.03 KB | sun |
#9 | drupal8.test-ua.9.patch | 4.37 KB | sun |
#8 | drupal8.test-ua.8.patch | 4.99 KB | sun |
#6 | drupal8.test-ua.6.patch | 5.33 KB | sun |
Comments
Comment #1
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedTriaging issues. Here's a patch.
Comment #2
sunMy stance remains to be the same:
The User Agent HTTP header is almost always set for all requests on production sites. PCREs, and especially non-trivial ones like this, are CPU-wise not free. The single, statically cached FALSE value is totally negligible compared to that.
My argument would potentially become moot if we'd statically cache some larger data, but as long as it's a single Boolean, I'm confident that you can show a performance decrease in benchmarks whereas the memory consumption remains identical.
Comment #3
sunsorry, didn't intend to change status
Comment #4
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedTempted to won'tfix, then. Note that this also wouldn't be one of the statics where WSCCI kernel subrequests would have a problem, probably.
Comment #5
sunComing from #1786402: SimpleTestTest is broken
There are 9 calls to drupal_valid_test_ua(), and among those, only the following are relevant:
config_get_config_directory():
- Called 2 times from drupal_container() on every request.
_drupal_initialize_db_test_prefix():
- Called 1 time from drupal_bootstrap() on every request.
drupal_system_listing():
- Called from drupal_get_filename() on a (static) cache miss.
- Called from _system_rebuild_module_data() on a (static) cache miss in system_rebuild_module_data().
- Called from _system_rebuild_theme_data() via system_rebuild_theme_data() (no static cache involved).
This essentially means that the PCRE matching + filectime() + fileinode() + drupal_hmac_base64() are guaranteed to be executed at minimum 3 times for every request.
If that is acceptable from a performance perspective, then I would prefer to remove this static variable instead of doing #1786402: SimpleTestTest is broken
Comment #6
sunWork on #1807662: Built-in APCu support in core (PHP 5.5 only) revealed that we have to remove this static cache, because it conflicts with unit tests.
Comment #8
sunRe-rolled against HEAD.
Comment #9
sunRe-rolled against HEAD.
Comment #11
sunFinally figured out what's going wrong:
There's no request involved in unit tests, so
drupal_valid_test_ua()
always returned FALSE. This mismatch also causedconfig_get_config_directory()
to not return the test-specific config directory.Attached patch should hopefully come back green.
Comment #13
Cyberwolf CreditAttribution: Cyberwolf commentedWent by #911354: Tests in profiles/[name]/modules cannot be run and cannot use a different profile for running tests, which does not seem to have solved the issues I am encountering with running contributed module tests when the modules are located in the install profile on D7, and then came over from #1786402: SimpleTestTest is broken to here. The drupal_valid_test_ua() call in drupal_system_listing() returns FALSE, as the HTTP request causing the setUp() method of my test case (extending DrupalWebTestCase) to be executed, seems to be done by my browser and not by the simpletest user agent.
I removed the conditional with drupal_valid_test_ua(), I do not think the variable that is set by simpletest is used anywhere else so I guess relying on it is fine.
Uploading my patch in case anyone else is encountering issues with this on D7.
Comment #14
sunThis idea is obsoleted by #2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead
Comment #15
pfrenssen#2171683: Remove all Simpletest overrides and rely on native multi-site functionality instead only covers D8, this still is a problem in D7.
Comment #16
sun