Problem/Motivation

We deprecated TestBase in #2847678: Deprecate WebTestBase and its required classes in favour of BrowserTestBase but

  • \Drupal\simpletest\TestBase::filePreDeleteCallback
  • \Drupal\simpletest\TestBase::insertAssert

are still used outside this class in run-tests.sh

And \Drupal\simpletest\TestBase::deleteAssert() exists. I don't think we should do anything about this method. It doesn't make sense - you should never delete an assert on itself - why would you do that?

Proposed resolution

Move \Drupal\simpletest\TestBase::insertAssert somewhere else.
Use \Drupal\Tests\BrowserTestBase::filePreDeleteCallback instead of \Drupal\simpletest\TestBase::filePreDeleteCallback

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Status: Active » Needs review
FileSize
6.37 KB
Lendude’s picture

Status: Needs review » Needs work
+++ b/core/modules/simpletest/simpletest.module
@@ -564,6 +563,63 @@ function simpletest_log_read($test_id, $database_prefix, $test_class) {
+  return self::getDatabaseConnection()

this self:: is not going to work here :)

Other then that, this indeed seems like the most straight forward way of fixing this

alexpott’s picture

Status: Needs work » Needs review
FileSize
559 bytes
6.37 KB

Fixing the fail... might write a test.

Status: Needs review » Needs work

The last submitted patch, 4: 3056877-4.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
851 bytes
6.13 KB

Okay cool at least we know that if this code is broken stuff breaks. So we can't properly deprecate but hey we can't properly deprecate TestBase either so /shrug.

Status: Needs review » Needs work

The last submitted patch, 6: 3056877-6.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.32 KB
5.2 KB

So there are situation where TestBase is loaded but not simpletest.module. Nice.

alexpott’s picture

+++ b/core/scripts/run-tests.sh
@@ -747,7 +747,7 @@ function simpletest_script_execute_batch($test_classes) {
-          TestBase::insertAssert($child['test_id'], $child['class'], FALSE, $message, 'run-tests.sh check');
+          simpletest_insert_assert($child['test_id'], $child['class'], FALSE, $message, 'run-tests.sh check');

We know that simpletest.module is loaded here because we inject the simpletest module into the test kernel - see \Drupal\Core\Test\TestRunnerKernel::__construct()

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

Rechecked and all the fails in #6 are in the remaining Simpletest. It is of little consequence that they keep their dependence on \Drupal\simpletest\TestBase::insertAssert, so keeping \Drupal\simpletest\TestBase::insertAssert in tact for that sound like a fine work around for me.

alexpott’s picture

I checked all the other static functions in classes that extend from TestBase and we have some usage of \Drupal\system\Tests\Update\DbUpdatesTrait so fixing that here.
There are also some other things that should be moved or refactored out of src/Tests:

  • \Drupal\views\Tests\ViewTestData
  • \Drupal\system\Tests\JsMessageTestCases
alexpott’s picture

Title: Handle the statics on TestBase » Remove usage of TestBase static functions because they are deprecated
alexpott’s picture

Created the followups:
#3057289: Move JsMessageTestCases out of src/Tests
#3057288: Move ViewTestData out of src/Tests

Well they're not really followups - more like things noticed along the way.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 3056877-11.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Reviewed & tested by the community

The fail is something happening on testbot.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 3056877-11.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
5.2 KB

Ah interesting. So the move of DbUpdatesTrait is not really correct. Interesting problem because this code needs to be autoloadable in the regular Drupal runtime because it is used in test module's hook_install(). So let's file a follow-up to deal with that and go back to the rtbc #8

alexpott’s picture

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/simpletest/simpletest.module
@@ -564,6 +563,63 @@ function simpletest_log_read($test_id, $database_prefix, $test_class) {
+  // We can't use storeAssertion() because this method is static.

The comment is no longer relevant since we're not in the class with the storeAssertion() method any more.

Lendude’s picture

Status: Needs work » Needs review
FileSize
413 bytes
5.13 KB

Removed the comment, changing it to say storeAssertion is protected seems like stating the obvious.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @Lendude +1 to removing the comment

  • catch committed 8426bf5 on 8.8.x
    Issue #3056877 by alexpott, Lendude: Remove usage of TestBase static...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 8426bf5 and pushed to 8.8.x. Thanks!

Status: Fixed » Closed (fixed)

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