Introduce TestRun objects and refactor TestDatabase to be testable

One things that pops up looking at TestDatabase right now is that it is doing many things in the same class. In particular, it
- sets up the database for the SUT, and
- manages (a part) of the test run where it logs test results, tracks db prefixes, keeps the {simpletest*} schema definitions, etc within the SITE database

The second part, though, it is still all over the place, see the table below to see which classes/scripts are doing what with the {simpletest*} tables:

One initial idea here could be to try and have a class (say TestTracker?) abstracting away the DB implementation details from the consuming code, and providing them a set of necessary methods. This way in the future the structure of the {simpletest*} tables could become totally internal (actually, maybe we won't even need db at the end of the day).

Here's an initial idea - quite rough, but I thought I share this at this point to get some feedback if this all makes any sense.

In general, with the table {simpletest_test_id} we trace the status of the last db prefix used by a test_id, and with {simpletest} we log the messages of the test execution.

We have already a TestStatus class, even if it has only a static method to map test status codes to some human readble status strings.

If we change that class to become an object that bears the status of a test and of the logged messages, and manage within that class all the interaction with the database, then we can abstract away the DB API calls from consuming code. Here I am only trying with 'insert' and 'update' operations - for 'delete' it's better wait for #2800267: Turn simpletest_clean*() functions into a helper class, and for 'select' wait for #3075648: Refactor TestDatabase::lastTestGet to use dynamic queries, and further feedback here.

Actually, potentially even the schema for creating the tables could be advocated by the class itself, so creating the tables on-demand rather than at simpletest install or by run-test.sh.

Update after commit of #3075648: Refactor TestDatabase::lastTestGet to use dynamic queries.

I thought here to add a new TestRun class, instead of expanding TestStatus. The idea is that a TestRun object bears a status and a message log, both being stored in db tables as the test run goes. The object abstracts away all the interaction with the tables. Here doing that for 'select', 'insert' and 'update' DB calls. Leaving out 'delete' ATM while waiting for #2800267: Turn simpletest_clean*() functions into a helper class.

This approach moves along the lines of what @larowlan was suggesting in #3075648-8: Refactor TestDatabase::lastTestGet to use dynamic queries and #3074043-19: Move simpletest.module DB-related functions to TestDatabase, deprecate:

+++ 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']);

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

since here $db_prefix = TestDatabase::lastTestGet($child['test_id'])['last_prefix'] becomes $test_run->getDatabasePrefix().

Interdiff would not make much sense.

Addressing test failures.

Doing some better usage of the TestRun object around. Haven't seen #10 yet. @Mile23 feel free to take over anytime, just pls assign to yourself so I know I will have to keep hands away :)

#10 sounds great!

I think we should separate concerns and have a value-only results class that consumes JUnit results, and then an abstract report maker, and then a concrete report maker that knows how to write to a Connection with a {simpletest} schema.

... and another implementation of the concrete report maker that, for example, just writes to memory for testing purposes without any DB interaction, if I understand correctly.

For now, in this patch I am just moving TestDatabase::logRead to a method on TestRun, since AFAICS it belongs there - that method, given a test run, scans the PHP error log file and adds appropriate log entries to the test run. At the end of the game, TestDatabase will only provide methods to setup and provide references for the database for the SUT, so to decouple that from managing the test results.

Assigning, trying to separate TestRun from results storage

So, here's my next idea.

Test runners (simpletest.module and run-test.sh) run tests opening TestRun objects that have state and an execution log. TestDatabase objects only cares about setting up the SUT database, not the results logging. Each TestRun object is associated with a TestRunResultsStorageInterface object, which responsibility is to store permanently the state and execution log of each test run. This interface is storage agnostic, implementations could use a database or anything else. SimpletestTestRunResultsStorage is at the moment the only implementation of TestRunResultsStorageInterface, which is using the database and the legacy {simpletest} and {simpletest_test_id} tables for the purpose.

Addressing test failures

Let's see this.

I suggesto to wait for #2800267: Turn simpletest_clean*() functions into a helper class before continuing.

Rerolled and moved TestDatabase::processPhpUnitResults to TestRun::processPhpUnitResults

Addressing test failure.

Refactoring run-tests.sh to use the new objects.

Patch in #25 is pushing too far.

#2800267: Turn simpletest_clean*() functions into a helper class is in and this now needs reroll + incorporating into SimpletestTestRunResultsStorage all the db 'delete' operations. Will work on that.

Just a reroll now.

Addressing failures of #28.

Here with SimpletestTestRunResultsStorage managing *all* interactions with the DB layer, AFAICS. This completes abstraction of the test results storage from the media actually used for it. Did some local testing, let's see what bot thinks.

Better.

Far from being ready, but I think we can unpostpone now, for review.

Working at adding tests for SimpletestTestRunResultsStorage and TestRun.

Adding a bunch of tests for SimpletestTestRunResultsStorage and TestRun.

Adjusted TestRun::processPhpErrorLogFile to make it testable and added tests + fixture test error.log file.

I do not like the results of the processing of the error.log file - a results record is added for each line of the file, and that means an entry for each item in the backtrace of the failure. In practice, given the test error.log file I'd expect 2 entries, but actually 9 entries are added. But that is pre-existing behavior, I just reflect it in the test. I'd suggest a follow-up to improve that logic.

Reroll after #3080482: Decouple FunctionalTestSetupTrait from the simpletest module

Adding a TestDatabase::getPhpErrorLogPath method to make finding the error.log file reusable and testable, adjusting tests.

Do we really need the path to be absolute?

Reroll after #3074433: Decouple run-tests.sh from the simpletest module

Adding a test for TestRun::processPhpUnitResults. Would be good to get some feedback about the direction of this patch.

Updated IS with (more or less) what the latest patches are trying to implement.

Some more cleanup+docs

Less typehints, they fail on Simpletest.

Moving TestRun::processPhpUnitResults into the PhpUnitTestRunner class.

Addressing failures of #54.

• Adjusting EnvironmentCleaner to use TestRun objects
• Deprecating TestSetupTrait::getDatabaseConnection so to channel all interaction to test results storage
• Simpletest's TestBase::deleteAssert to use test results storage instead of direct db calls

Let's see how this works now after removal of simpletest from core in #3110862: Remove simpletest module from core. Reroll.

I think this becomes 9.1.x material, now.

Reintroducing TestDatabase::testingSchema to stay away from changing Simpletest stub

Reroll.

Rerolled and adding typehints to functions.

Rerolled.

It feels like Simpletest is abandoned, there is no real activity in the contrib module, so if this gets us on the way to removing Simpletest-isms from core then I think it's a good idea.

1. +++ b/core/lib/Drupal/Core/Test/TestDatabase.php
   @@ -316,117 +153,21 @@ public function logRead($test_id, $test_class) {
   +   * @todo deprecate?
   

   This is considered internal so we could just remove it?

2. +++ b/core/lib/Drupal/Core/Test/TestSetupTrait.php
   @@ -108,9 +108,15 @@
   +    @trigger_error(__METHOD__ . ' in drupal:x.x.x and is removed from drupal:y.y.y. There is no replacement. See https://www.drupal.org/node/xxx', E_USER_DEPRECATED);
   

   Might as well set these versions and add a change record.

Otherwise I don't see any big issues and I think this is a nice cleanup.

Thank you @longwave, I am on it.

Drafted CR https://www.drupal.org/node/3176816

Addressed #67, thank you.

Fixes for failures in #70.

A few more real tiny nits now I have gone back for a second look:

1. +++ b/core/lib/Drupal/Core/Test/PhpUnitTestRunner.php
   @@ -226,19 +226,37 @@ public function runTests($test_id, array $unescaped_test_classnames, &$status =
   +  public function processPhpUnitResults(TestRun $test_run, array $phpunit_results): void {
   +    if (!empty($phpunit_results)) {
   +      foreach ($phpunit_results as $result) {
   

   As we typehint to array the empty check seems useless, foreach over empty array will do nothing anyway.

2. +++ b/core/lib/Drupal/Core/Test/SimpletestTestRunResultsStorage.php
   @@ -0,0 +1,302 @@
   +   * This method is introduced for BC.
   

   Is this actually needed?

3. +++ b/core/lib/Drupal/Core/Test/SimpletestTestRunResultsStorage.php
   @@ -0,0 +1,302 @@
   +          'description' => 'Primary Key: Unique simpletest ID used to group test results together. Each time a set of tests
   +                            are run a new test ID is used.',
   

   I don't think we normally break long lines like this.

4. +++ b/core/scripts/run-tests.sh
   @@ -623,6 +632,15 @@ function simpletest_script_setup_database($new = FALSE) {
   +function script_setup_test_run_results_storage($new = FALSE) {
   

   All other functions in run-tests.sh begin with simpletest_script_ except this one, probably should be consistent.

5. +++ b/core/scripts/run-tests.sh
   @@ -894,7 +906,7 @@ function simpletest_script_command($test_id, $test_class) {
   -  $command .= " --test-id $test_id";
   +  $command .= " --test-id " . $test_run->id();
   

   This can be single-quoted now.

#73 all done, thank you.

#73.4 I'd rather do the other way round i.e. replace "simpletest" with "runtests" everywhere but yea, let's do that elsewhere.

Looks great now, thanks! Looking forward to some followups where we remove more references to Simpletest :)

Rerolled

@mondrake

I think we need to rebase the MR on 9.3.x.
(And by we, I of course mean: you, because only the original creator of an MR can do so 😇)

After sitting for 9 months in RTBC with no action, I'm not sure where this issue is going, really. But OK, rebasing.

I hear what you're saying there and I was kinda hoping a rebase would get this one in quicker.

Thanks :)

Back to RTBC per #75, trusting TestBot to kick it down when it's not returning green.

This issue has been sitting in RTBC for over one year with no sign of interest from committers. I assume won't fix, if nothing else to avoid rerolling it to no avail.

Discussed with @longwave in Slack it's worth giving this another try. I will rebase this onto 10.1.x.

Rebased onto 10.1.x

A couple of questions/comments - I am OK if you want to defer further changes to followups, getting this testable means we can refactor and untangle the Simpletest-isms more easily.

Please see comments inline the MR.

Thanks - this looks OK to me and will mean that we can start removing more dead code related to Simpletest in followups.

I've stared at this issue lots of times and little bits of it make me feel "are we sure that we need to do this or that" - for example I'm not sure about the utility of having an interface for TestRunResultsStorageInterface - I get that internally we're going to swap it for a non-simpletest labelled implementation at some point in the future but it feel unnecessary. That said, I'm not sure that these objections are that important or should stand in the way of progress here. I don't think we should consider any of this public API and the MR helpfully adds @internal in quite a few places. Therefore I think we should proceed here with a commit.

Committed f711435 and pushed to 10.1.x. Thanks!

Thanks. BTW, there's a bit of Space Oddity in Drupal's code, now :)