Problem/Motivation

These functions in the simpletest module deal with the TestDatabase:

  • simpletest_insert_asssert()
  • simpletest_last_test_get()
  • simpletest_log_read()
  • simpletest_schema()

If we want to turn the simpletest module into a contrib module, we'll need to make sure their behavior is still in core so the test runner can use them.

Proposed resolution

Move the behavior of these functions to be methods on the Drupal\Core\Test\TestDatabase class.

Deprecate the functions for removal.

The special case is simpletest_schema(), which is an implementation of hook_schema(). This function is used by both the simpletest module (in a hook context) and the run-tests.sh script (not in a hook context).

Since it's used this way, it can also be a method on TestDatabase, which is then called by simpletest_schema().

Since simpletest_schema() is a hook, it won't be deprecated.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#24 interdiff.txt450 bytesMile23
#24 3074043_24.patch20.67 KBMile23
#22 interdiff.txt1.43 KBMile23
#22 3074043_22.patch20.67 KBMile23
#16 interdiff.txt2.17 KBMile23
#16 3074043_16.patch20.58 KBMile23
#14 interdiff.txt1.74 KBMile23
#14 3074043_14.patch20.84 KBMile23
#10 interdiff.txt5.11 KBMile23
#10 3074043_10.patch19.19 KBMile23
#8 interdiff.txt1.52 KBMile23
#8 3074043_8.patch19.15 KBMile23
#6 interdiff.txt4.18 KBMile23
#6 3074043_6.patch20.66 KBMile23
#3 3074043_3.patch18.33 KBMile23
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23 created an issue. See original summary.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Active » Postponed

Postponed on decisions made in #3057420: [meta] How to deprecate Simpletest with minimal disruption

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Postponed » Needs review
FileSize
3074043_3.patch18.33 KB

Early try.

Still mainly waiting on consensus from #3057420: [meta] How to deprecate Simpletest with minimal disruption so this will go back to being postponed after the testbot starts.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs review » Postponed
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Postponed » Needs review

Unpostponing based on #3057420-17: [meta] How to deprecate Simpletest with minimal disruption

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
FileSize
3074043_6.patch20.66 KB
interdiff.txt4.18 KB

Draft change record: https://www.drupal.org/node/3075252

Updating deprecation messages, adding deprecation test.

Status: Needs review » Needs work

The last submitted patch, 6: 3074043_6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Needs review
FileSize
3074043_8.patch19.15 KB
interdiff.txt1.52 KB

Removed the deprecation test because there is no way to mock TestDatabase. Like, at all. :-)

Fixing that problem is not in scope here, but it is why these deprecated functions do not have any tests.

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
CreditAttribution: voleger as a volunteer and at GOLEMS GABB, Drupal Ukraine Community for Drupal Ukraine Community, GOLEMS GABB commented
Status: Needs review » Needs work 
  1. +++ b/core/lib/Drupal/Core/Test/TestDatabase.php
@@ -179,4 +179,213 @@ protected function getLockFile($lock_id) {
+   * the method behaves just like \Drupal\simpletest\TestBase::assert() in terms
...
+   *   The an array containing the keys 'file' and 'line' that represent the file

    CS line length issue 

  2. +++ b/core/lib/Drupal/Core/Test/TestDatabase.php
@@ -179,4 +179,213 @@ protected function getLockFile($lock_id) {
+   * @return

    Return type missing 

  3. +++ b/core/lib/Drupal/Core/Test/TestDatabase.php
@@ -179,4 +179,213 @@ protected function getLockFile($lock_id) {
+    return static::getConnection()
+        ->insert('simpletest')
+        ->fields($assertion)
+        ->execute();

    Fix indentation 

  4. +++ b/core/lib/Drupal/Core/Test/TestDatabase.php
@@ -179,4 +179,213 @@ protected function getLockFile($lock_id) {
+   * @param $test_id

    Param type missing 

  5. +++ b/core/lib/Drupal/Core/Test/TestDatabase.php
@@ -179,4 +179,213 @@ protected function getLockFile($lock_id) {
+   * @return array

    An empty line before @return missing 

  6. +++ b/core/modules/simpletest/simpletest.module
@@ -507,19 +507,15 @@ function _simpletest_batch_finished($success, $results, $operations, $elapsed) {
+ * @deprecated in Drupal 8.8.0 for removal before 9.0.0. Use
+ *   \Drupal\Core\Test\TestDatabase::lastTestGet() instead.

    CS issue with deprecation message 

  7. +++ b/core/modules/simpletest/simpletest.module
@@ -507,19 +507,15 @@ function _simpletest_batch_finished($success, $results, $operations, $elapsed) {
+  @trigger_error(__FUNCTION__ . ' is deprecated in Drupal 8.8.0 for removal before Drupal 9.0.0. Use \Drupal\Core\Test\TestDatabase::lastTestGet() instead. See https://www.drupal.org/node/3075252', E_USER_DEPRECATED);

    CS issue with an error message 

  8. +++ b/core/modules/simpletest/simpletest.module
@@ -537,30 +533,16 @@ function simpletest_last_test_get($test_id) {
+ * @deprecated in Drupal 8.8.0 for removal before 9.0.0. Use
+ *   \Drupal\Core\Test\TestDatabase::logRead() instead.
...
+  @trigger_error(__FUNCTION__ . ' is deprecated in Drupal 8.8.0 for removal before Drupal 9.0.0. Use \Drupal\Core\Test\TestDatabase::logRead() instead. See https://www.drupal.org/node/3075252', E_USER_DEPRECATED);

@@ -589,34 +571,15 @@ function simpletest_log_read($test_id, $database_prefix, $test_class) {
+ * @deprecated in Drupal 8.8.0 for removal before 9.0.0. Use
+ *   \Drupal\Core\Test\TestDatabase::insertAssert() instead.
...
+  @trigger_error(__FUNCTION__ . ' is deprecated in Drupal 8.8.0 for removal before Drupal 9.0.0. Use \Drupal\Core\Test\TestDatabase::insertAssert() instead. See https://www.drupal.org/node/3075252', E_USER_DEPRECATED);

    The same here

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Needs review
FileSize
3074043_10.patch19.19 KB
interdiff.txt5.11 KB

Thanks, @voleger.

Addressed CS issues in #9.

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
CreditAttribution: voleger as a volunteer and at GOLEMS GABB, Drupal Ukraine Community for Drupal Ukraine Community, GOLEMS GABB commented
Status: Needs review » Reviewed & tested by the community

+1 for RTBC

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/lib/Drupal/Core/Test/TestDatabase.php
@@ -179,4 +179,214 @@ protected function getLockFile($lock_id) {
+  /**
+   * Implements hook_schema().
+   */
+  public static function simpletestSchema() {

This is no longer a hook_schema implementation, and I think we should now overcome 'simpletest' being part of the method name - how about just getSchema?

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented 
+++ b/core/lib/Drupal/Core/Test/TestDatabase.php
@@ -179,4 +179,214 @@ protected function getLockFile($lock_id) {
+  public static function lastTestGet($test_id) {
+    $last_prefix = static::getConnection()
+      ->queryRange('SELECT last_prefix FROM {simpletest_test_id} WHERE test_id = :test_id', 0, 1, [
+        ':test_id' => $test_id,
+      ])
+      ->fetchField();
+    $last_test_class = static::getConnection()
+      ->queryRange('SELECT test_class FROM {simpletest} WHERE test_id = :test_id ORDER BY message_id DESC', 0, 1, [
+        ':test_id' => $test_id,
+      ])
+      ->fetchField();

I'd also love these to become dynamic queries instead (i.e. $connection->select(...) etc.) but I guess that's an obsessions of mine only and it would be out-of-scope here anyway.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Needs review
FileSize
3074043_14.patch20.84 KB
interdiff.txt1.74 KB

#12: Fair point on the docblock. I think simpletestSchema() is a good name because it's the simpletest schema. But let's call it testingSchema(). getSchema() might imply a few other APIs.

#13: Yes, a bit out of scope, but you're welcome to file a follow-up and I'll even review it. :-)

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs review » Needs work

Thanks.

+++ b/core/lib/Drupal/Core/Test/TestDatabase.php
@@ -179,4 +179,217 @@ protected function getLockFile($lock_id) {
diff --git a/core/modules/media_library/src/Plugin/CKEditorPlugin/DrupalMediaLibrary.php b/core/modules/media_library/src/Plugin/CKEditorPlugin/DrupalMediaLibrary.php

diff --git a/core/modules/media_library/src/Plugin/CKEditorPlugin/DrupalMediaLibrary.php b/core/modules/media_library/src/Plugin/CKEditorPlugin/DrupalMediaLibrary.php
index 44aadf5408..74d81af95c 100644

index 44aadf5408..74d81af95c 100644
--- a/core/modules/media_library/src/Plugin/CKEditorPlugin/DrupalMediaLibrary.php

--- a/core/modules/media_library/src/Plugin/CKEditorPlugin/DrupalMediaLibrary.php
+++ b/core/modules/media_library/src/Plugin/CKEditorPlugin/DrupalMediaLibrary.php

+++ b/core/modules/media_library/src/Plugin/CKEditorPlugin/DrupalMediaLibrary.php
+++ b/core/modules/media_library/src/Plugin/CKEditorPlugin/DrupalMediaLibrary.php
@@ -125,7 +125,7 @@ public function getConfig(Editor $editor) {

@@ -125,7 +125,7 @@ public function getConfig(Editor $editor) {
       'media_library.opener.editor',
       $media_type_ids,
       reset($media_type_ids),
-      1,
+      -1,
       ['filter_format_id' => $editor->getFilterFormat()->id()]
     );
 
diff --git a/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php

diff --git a/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
index 3daea82dcd..d6d3270dba 100644

index 3daea82dcd..d6d3270dba 100644
--- a/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php

--- a/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
+++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php

+++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
+++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
@@ -164,9 +164,7 @@ public function testButton() {

@@ -164,9 +164,7 @@ public function testButton() {
       $this->assertSame($expected_tab_order[$key], $tab->getText());
     }
 
-    $assert_session->pageTextContains('0 of 1 item selected');
     $assert_session->elementExists('css', '.media-library-item')->click();
-    $assert_session->pageTextContains('1 of 1 item selected');
     $assert_session->elementExists('css', 'button.media-library-select.button.button--primary')->click();
     $this->assignNameToCkeditorIframe();
     $this->getSession()->switchToIFrame('ckeditor');

These look like leaks from another patch.

I wonder whether moving these methods to TestDatabase doesn't require also some tests to be added to TestDatabaseTest?

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Needs review
Issue tags: +Needs followup
FileSize
3074043_16.patch20.58 KB
interdiff.txt2.17 KB

In an ideal world I'd be able to test the new methods. But because TestDatabase is almost entirely static, we can't mock anything and we'd end up changing test results. It's the same reason we can't write a deprecation test for the functions we're deprecating.

I expanded TestDatabaseTest a little bit, for a single test case, but that's as far as we can go.

We should make a follow-up to refactor this stuff so it's testable. Right now the focus is deprecating the functions.

Mile23’s picture

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

And there's the follow-up.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs review » Reviewed & tested by the community
Related issues: +#3075648: Refactor TestDatabase::lastTestGet to use dynamic queries

Filed #3075648: Refactor TestDatabase::lastTestGet to use dynamic queries as followup to #13/#14.

There's a small CS issue that could also be fixed on commit.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Needs review 
  1. +++ b/core/lib/Drupal/Core/Test/TestDatabase.php
@@ -179,4 +179,218 @@ protected function getLockFile($lock_id) {
+    return [$last_prefix, $last_test_class];

    We have a chance to rework this here. I doubt this is what we'd design if we did it from scratch. While we're touching it - should we make this a value object return? We can juggle back to the array format it in the old function for BC reasons.

    An API is forever and all that. 

  2. +++ b/core/modules/simpletest/simpletest.module
@@ -507,19 +507,15 @@ function _simpletest_batch_finished($success, $results, $operations, $elapsed) {
+  @trigger_error(__FUNCTION__ . ' is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use \Drupal\Core\Test\TestDatabase::lastTestGet() instead. See https://www.drupal.org/node/3075252', E_USER_DEPRECATED);

@@ -537,30 +533,16 @@ function simpletest_last_test_get($test_id) {
+  @trigger_error(__FUNCTION__ . ' is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use \Drupal\Core\Test\TestDatabase::logRead() instead. See https://www.drupal.org/node/3075252', E_USER_DEPRECATED);

@@ -589,34 +571,15 @@ function simpletest_log_read($test_id, $database_prefix, $test_class) {
+  @trigger_error(__FUNCTION__ . ' is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use \Drupal\Core\Test\TestDatabase::insertAssert() instead. See https://www.drupal.org/node/3075252', E_USER_DEPRECATED);

    should we be adding deprecation tests for these?

Mile23’s picture

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

We can't add deprecation tests because TestDatabase is static, and would write to our test results. That's why there's a followup: #3075608: Introduce TestRun objects and refactor TestDatabase to be testable

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Should we mark all the methods as @internal if we hope to get rid of them then?

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Related issues: +#2946439: TestDatabase should always lock
FileSize
3074043_22.patch20.67 KB
interdiff.txt1.43 KB

It's a can of worms, because we clearly need to refactor how this works for maintainability. But we've been trying to get that done for a few years now. @mondrake just filed #3075648: Refactor TestDatabase::lastTestGet to use dynamic queries and I filed #3075608: Introduce TestRun objects and refactor TestDatabase to be testable, we've reworked the schema a little in #2905007: Allow run-tests.sh to report skipped/risky/incomplete PHPUnit-based tests, @alexpott wants to force the DB to always lock in #2946439: TestDatabase should always lock, etc.

In fact that last one is your best chance to see refactoring and deprecation before 9.0.0. (For instance: TestDatabase already had too many responsibilities before we added these methods: Finding the results database in config and also managing locks.)

I think marking all the new methods as internal is an excellent idea.

Sigh. Still no official CS for @internal.

mondrake’s picture

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

This is doing the moves, and we have follow-ups for the refactors. RTBC (there's a small CS 'Expected 1 blank line after function; 2 found' issue but easily fixable at commit). RTBC

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
FileSize
3074043_24.patch20.67 KB
interdiff.txt450 bytes

CS issue from #23. Leaving RTBC for minor CS change.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

  • larowlan committed 0041ba0 on 8.8.x 
    Issue #3074043 by Mile23, mondrake, voleger: Move simpletest.module DB-...
larowlan’s picture

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

Committed 0041ba0 and pushed to 8.8.x. Thanks!

Published change record

Status: Fixed » Closed (fixed)

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