curlConnect() currently requests the front page at the beginning of each session, i.e. the first call to drupalGet() or drupalPost() in every test actually results in two requests.

AFAICT there is no need to do this. I assume this is a workaround for #293612: user_authenticate() should work when $_COOKIE is empty that has now been fixed (can anybody confirm this?). Only one test relies on this (Bootstrap > Page cache test), and that is easily fixed.

This patch removes the call to curlExec() from curlConnect(). curlConnect() is kept as a separate function but is renamed to curlInitialize(), because it no longer connects to the server.

I expect this to make the tests run faster, though I don't know whether it is noticeable.

CommentFileSizeAuthor
#23 curlConnect-3.patch5.66 KBc960657
#19 curlConnect-2.patch5.69 KBc960657
curlConnect-1.patch4.17 KBc960657
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

c960657’s picture

c960657 CreditAttribution: c960657 commented
Title: Simpletest speedup: skip call to curlExec() in curlConnect() » Skip call to curlExec() in curlConnect()
Priority: Critical » Normal

I have no idea why this doesn't pass in the testbed. All tests pass at my box. The Temporary query test doesn't even use the cURL framework.

Edit: Sorry, it actually does.

c960657’s picture

c960657 CreditAttribution: c960657 commented
Status: Needs work » Needs review
chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Reviewed & tested by the community

As the person who written this code I can certainly confirm that this was such a workaround. And the tests now pass.

c960657’s picture

c960657 CreditAttribution: c960657 commented

I just requested a re-test of the same patch, so it must have been a glitch in the testing robot.

Without the patch: Passed: 7294 passes, 0 fails, 0 exceptions
With the patch: Passed: 7149, 0 fails, 0 exceptions

This indicates that the number of HTTP requests is reduced by 145 (each extra call to curlExec() generates one assertion).

chx’s picture

chx CreditAttribution: chx commented
Title: Skip call to curlExec() in curlConnect() » Simpletest speedup: skip call to curlExec() in curlConnect()
Priority: Normal » Critical
Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

Tested and works for me. Good improvement!

boombatower’s picture

boombatower CreditAttribution: boombatower commented

Nice work.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

I can't reproduce the exceptions either.

boombatower’s picture

boombatower CreditAttribution: boombatower commented
Status: Needs work » Reviewed & tested by the community
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs review

Hm. Unfortunately, I can't commit this if it causes the testing bot to find failures/exceptions or we'll run into the same situation as over the weekend where ALL patches are marked CNW.

Let's wait to hear back from the testing bot. If it comes back green, feel free to mark back to RTBC.

Status: Needs review » Needs work

The last submitted patch failed testing.

c960657’s picture

c960657 CreditAttribution: c960657 commented
Status: Fixed » Needs review

Hmm, it appears that there is some problem with the patch or with the temporary query test. It is hard to debug without access to the error messages from the testbed. The attached patch tries to send some debug output back to me (don't know whether this is somehow blocked - we'll see). Do not review this patch.

The testbed did not reproduce the error in all runs, so it may be e.g. a timing issue or a race condition when several concurrent tests are run (I don't know exactly how the testbed works). I have tried running this test 20-30 times without seeing the problem. I have tried with and without $db_prefix, with and without $base_path, with PHP running as CGI and Apache 2 module, without any difference.

queryTemporary() uses a Engine=MEMORY table - I wonder whether that is subject to any memory constraints?
The temporary table is limited to the current database connection. Is there any chance that Drupal will reconnect to the database and thus use another connection between the two db calls in database_test_db_query_temporary().

[edit: patch removed]

Title: Skip call to curlExec() in curlConnect() » Simpletest speedup: skip call to curlExec() in curlConnect()
Priority: Normal » Critical

The last submitted patch failed testing.

c960657’s picture

c960657 CreditAttribution: c960657 commented
Status: Needs work » Needs review

This is the error that happens in the testbed:

Message
CREATE TEMPORARY TABLE temporary Engine=MEMORY SELECT status FROM {system} -
Array
(
)
SQLSTATE[42000]: Syntax error or access violation: 1044 Access denied for user 'drupal'@'localhost' to database 'drupal6_checkout'
Group
PDOException
Caller
database_test_db_query_temporary()
/var/www/html/drupal6/sites/default/files/checkout/modules/simpletest/tests/database_test.module
line 58

Is the MySQL user missing the Create_tmp_table_priv on the testbed machine?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Sounds very likely. Opened #337761: Slaves need MySQL create temporary tables permission.

Status: Needs review » Needs work

The last submitted patch failed testing.

c960657’s picture

c960657 CreditAttribution: c960657 commented
Status: Needs work » Needs review
FileSize
curlConnect-2.patch5.69 KB

Reroll of original patch (without debug output) for HEAD. It includes an update to the test that was just added #323474: hook_boot() not invoked on uncached page views if cache mode is AGGRESSIVE.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Needs review » Needs work

The new test in bootstrap.test for hook_boot and hook_exit would need to be changed with this patch, since it accounts for two requests in the first call to drupalGet:

  function testHookBootExit() {
    // Test with cache disabled. Boot and exit should always fire. Initialize
    // the number of hook calls to one since the first call to two since the
    // first call to drupalGet actually performs two HTTP requests.
    variable_set('cache', CACHE_DISABLED);
    $this->drupalGet('');
    $calls = 2;
Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Needs work » Needs review

Cross-posted... :)

Status: Needs review » Needs work

The last submitted patch failed testing.

c960657’s picture

c960657 CreditAttribution: c960657 commented
FileSize
curlConnect-3.patch5.66 KB

Reroll (there was a follow-up commit in #323474).

c960657’s picture

c960657 CreditAttribution: c960657 commented
Status: Needs work » Needs review
boombatower’s picture

boombatower CreditAttribution: boombatower commented

Please do not commit until #337761: Slaves need MySQL create temporary tables permission. is marked as fixed.

Secondly is this in D7 INSTALL.txt?

Dries’s picture

Dries CreditAttribution: Dries commented

Committed to CVS HEAD. Nice find, c960657.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Needs review » Fixed

Committed to CVS HEAD. Nice find, c960657.

Status: Needs review » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.