Problem/Motivation

Follow-up to #3074043-13: Move simpletest.module DB-related functions to TestDatabase, deprecate

Check if it the queries can be changed to dynamic.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#6 3075648-6.patch5.1 KBmondrake
#6 interdiff_5-6.txt941 bytesmondrake
#5 3075648-5.patch4.7 KBmondrake
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Active » Postponed
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

Maybe a duplicate of #3075608: Introduce TestRun objects and refactor TestDatabase to be testable

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Title: [PP-1] Refactor TestDatabase::lastTestGet to use dynamic queries » Refactor TestDatabase::lastTestGet to use dynamic queries
Assigned: Unassigned » mondrake
Status: Postponed » Active

#3074043: Move simpletest.module DB-related functions to TestDatabase, deprecate is in, unpostponing.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Assigned: mondrake » Unassigned
Status: Active » Needs review
FileSize
3075648-5.patch4.7 KB

#3 I do not think this is a duplicate of #3075608: Introduce TestRun objects and refactor TestDatabase to be testable, the scope of this is to just use dynamic query API in place of static queries within TestDatabase::lastTestGet.

Here's the idea: the Select API in the patch will produce the following SQL statement:

SELECT sttid.last_prefix AS last_prefix, st.test_class AS test_class
FROM
(SELECT MAX(message_id) AS max_message_id
FROM
{simpletest} sub
WHERE test_id = :db_condition_placeholder_0) st_sub
INNER JOIN {simpletest} st ON st.message_id = st_sub.max_message_id
INNER JOIN {simpletest_test_id} sttid ON st.test_id = sttid.test_id

which is doing the same job of the current two separate queries.

I am also changing TestDatabase::lastTestGet to return directly the associative array fetched from the DB.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
FileSize
interdiff_5-6.txt941 bytes
3075648-6.patch5.1 KB

I came across a pre-existing bug while testing this patch running simpletest test via run-tests.sh, when there are failing tests (yes, a bit of an edge case in core nowadays, but still...).

A $last_test_class is undefined in run-tests.sh, leading to notices being thrown in the CLI:
PHP Notice: Undefined variable: last_test_class in /home/travis/drupal8/core/scripts/run-tests.sh on line 936

Fixing here.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Nice catch on #6. The docblock for simpletest_script_cleanup() is wrong. It's weird that you can only clean up after PHPUnit by passing in --clean. That's a side-effect that's used by the testbot, though, so let's not work too hard to change it...

The rest LGTM.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/modules/simpletest/simpletest.module
@@ -490,8 +490,8 @@ function _simpletest_batch_finished($success, $results, $operations, $elapsed) {
+    (new TestDatabase($last_test['last_prefix']))->logRead($test_id, $last_test['test_class']);

@@ -515,7 +515,7 @@ function _simpletest_batch_finished($success, $results, $operations, $elapsed) {
+  return array_values(TestDatabase::lastTestGet($test_id));

+++ b/core/scripts/run-tests.sh
@@ -758,7 +758,7 @@ function simpletest_script_execute_batch($test_classes) {
+            $db_prefix = TestDatabase::lastTestGet($child['test_id'])['last_prefix'];

@@ -930,7 +931,7 @@ function simpletest_script_cleanup($test_id, $test_class, $exitcode) {
+    (new TestDatabase($db_prefix))->logRead($test_id, $last_test['test_class']);

I said the same thing over in [#13225478] - are we sure we don't want to use a value object here?

Happy for the answer to be no, this code is legacy

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Related issues: +#3075608: Introduce TestRun objects and refactor TestDatabase to be testable

I think @mondrake might be reaching a similar conclusion in #3075608-3: Introduce TestRun objects and refactor TestDatabase to be testable

I'd wager there's at least a slight performance benefit to one query instead of two... This code runs at least once per test class per drupalci build, so ~27,000 times.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs review » Reviewed & tested by the community

are we sure we don't want to use a value object here?

can I suggest we defer that discussion to #3075608: Introduce TestRun objects and refactor TestDatabase to be testable, where we are experimenting with an object that tracks the test run? There is a very initial patch for review there.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

Committed 5c326c9 and pushed to 8.8.x. Thanks!

  • larowlan committed 5c326c9 on 8.8.x 
    Issue #3075648 by mondrake: Refactor TestDatabase::lastTestGet to use...

Status: Fixed » Closed (fixed)

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