Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Niklas Fiekas’s picture

Status: Active » Needs review
FileSize
11.63 KB
1.05 KB

Triaging issues. Here's a patch.

sun’s picture

Status: Needs review » Needs work
+++ b/core/includes/bootstrap.inc
@@ -2366,12 +2366,6 @@ function drupal_container(ContainerBuilder $reset = NULL) {
   if (isset($_SERVER['HTTP_USER_AGENT']) && preg_match("/^(simpletest\d+);(.+);(.+);(.+)$/", $_SERVER['HTTP_USER_AGENT'], $matches)) {

My 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.

sun’s picture

Status: Needs work » Needs review

sorry, didn't intend to change status

Niklas Fiekas’s picture

Tempted to won'tfix, then. Note that this also wouldn't be one of the statics where WSCCI kernel subrequests would have a problem, probably.

sun’s picture

Priority: Minor » Normal
Issue tags: +Performance, +Testing system

Coming 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

sun’s picture

Assigned: Unassigned » sun
Issue tags: -Performance
FileSize
5.33 KB

Work 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.

Status: Needs review » Needs work

The last submitted patch, drupal8.test-ua.6.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
4.99 KB

Re-rolled against HEAD.

sun’s picture

FileSize
4.37 KB

Re-rolled against HEAD.

Status: Needs review » Needs work

The last submitted patch, drupal8.test-ua.9.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
8.03 KB

Finally 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 caused config_get_config_directory() to not return the test-specific config directory.

Attached patch should hopefully come back green.

Status: Needs review » Needs work

The last submitted patch, drupal8.test-ua.11.patch, failed testing.

Cyberwolf’s picture

FileSize
957 bytes

Went 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.

sun’s picture

Issue summary: View changes
Status: Needs work » Closed (duplicate)
pfrenssen’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Closed (duplicate) » Active
sun’s picture

Assigned: sun » Unassigned