Problem/Motivation

SimpleTest is no longer useful, everyone has migrated to PHPUnit.

There is still support code for SimpleTest tests in TestDiscovery and TestRunnerKernel (and probably some related classes).

Steps to reproduce

Proposed resolution

Remove code relating to SimpleTest-based tests from these classes.

Deprecation is not required as tests are considered internal and users have had the full Drupal 9 cycle to upgrade to PHPUnit.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

The SimpleTest module was moved to contrib prior to Drupal 9.0.0. Drupal 10 removes support for SimpleTest from the core test runner. Projects that use SimpleTest should convert their tests to PHPUnit.

CommentFileSizeAuthor
#16 interdiff.3254726.12-16.txt1.44 KBlongwave
#16 3254726-16.patch27.75 KBlongwave
#12 3254726-12.patch27.51 KBlongwave
#10 3254726-10.patch28.02 KBlongwave
#9 3254726-9.patch27.78 KBlongwave
#8 3254726-8.patch484 byteslongwave
#6 interdiff.3254726.2-6.txt10.84 KBlongwave
#6 3254726-6.patch27.78 KBlongwave
#3 3254726-2.patch17.83 KBlongwave
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented
Issue summary: View changes
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented
Status: Active » Needs review
FileSize
3254726-2.patch17.83 KB

TestDiscovery has support for parsing @dependencies in docblocks; is this a SimpleTest-ism that can be removed?

   *   - requires: An associative array containing test requirements parsed from
   *     PHPDoc annotations:
   *     - module: List of Drupal module extension names the test depends on.
..
    if (isset($annotations['dependencies'])) {
      $info['requires']['module'] = array_map('trim', explode(',', $annotations['dependencies']));
    }
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented

run-tests.sh can probably be cleaned up some more. I find this hard to edit in PhpStorm as it's convinced it is a shell script :)

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave commented

TestDiscovery::parseTestClassAnnotations() seems to be some kind of extra support for PHPUnit, but doesn't appear to be called.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
FileSize
3254726-6.patch27.78 KB
interdiff.3254726.2-6.txt10.84 KB

Re #3/#5 - PHPUnit uses @requires module to do this instead, TestRequirementsTrait deals with this now and TestDiscovery::parseTestClassAnnotations() appears to be dead code.

Addressed #4, cleaned up some more SimpleTest-only code from run-tests.sh.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Title: Remove SimpleTest support from TestDiscovery and TestRunnerKernel » Remove SimpleTest support from run-tests.sh, TestDiscovery and TestRunnerKernel
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
FileSize
3254726-8.patch484 bytes

Fix unused use statement.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
FileSize
3254726-9.patch27.78 KB

...and upload the patch instead of an interdiff.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
FileSize
3254726-10.patch28.02 KB

Third time lucky?

Status: Needs review » Needs work

The last submitted patch, 10: 3254726-10.patch, failed testing. View results

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Needs review
FileSize
3254726-12.patch27.51 KB

Two more test cases can be deleted if we don't support @dependencies.

Mile23’s picture

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

Anything that says 'remove simpletest support' in the title gets +1 from me.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Issue tags: +run-tests.sh
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs review » Needs work 
  1. +++ b/core/lib/Drupal/Core/Test/RunTests/TestFileParser.php
index cf0ca32f28..17912c9050 100644
--- a/core/lib/Drupal/Core/Test/TestDiscovery.php

--- a/core/lib/Drupal/Core/Test/TestDiscovery.php
+++ b/core/lib/Drupal/Core/Test/TestDiscovery.php

    Can we mark this class @internal? 

  2. +++ b/core/lib/Drupal/Core/Test/TestDiscovery.php
diff --git a/core/lib/Drupal/Core/Test/TestRunnerKernel.php b/core/lib/Drupal/Core/Test/TestRunnerKernel.php
index 2315693eac..f55cb67093 100644

index 2315693eac..f55cb67093 100644
--- a/core/lib/Drupal/Core/Test/TestRunnerKernel.php

--- a/core/lib/Drupal/Core/Test/TestRunnerKernel.php
+++ b/core/lib/Drupal/Core/Test/TestRunnerKernel.php

    Can we mark this class @internal?

  3. +++ b/core/scripts/run-tests.sh
@@ -687,7 +683,7 @@ function simpletest_script_setup_database($new = FALSE) {
+      simpletest_script_print_error('Missing Simpletest database schema. Use the --sqlite parameter.');

    maybe we can remove 'Simpletest' from the statement and just say sth like 'Missing database schema to collect test results' 

  4. +++ b/core/scripts/run-tests.sh
@@ -780,10 +776,6 @@ function simpletest_script_execute_batch($test_classes) {
-        // Free-up space by removing any potentially created resources.
-        if (!$args['keep-results']) {
-          simpletest_script_cleanup($child['test_id'], $child['class'], $status['exitcode']);
-        }

    Are we sure? I think we still need to have a way to allow testing artifacts to be stored somewhere... and currently that is all over the place (for instance, the HTML output is in a folder that is not touched by the cleanup IIRC). I suggest to leave this in ATM and look to streamline artifact management. But that will take longer and certainly involve infra.

EDIT: sorry I misread 4. It's OK.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Needs review
FileSize
3254726-16.patch27.75 KB
interdiff.3254726.12-16.txt1.44 KB

Thanks for reviewing, rerolled and addressed #15.

mondrake’s picture

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

Great

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/scripts/run-tests.sh
@@ -905,112 +860,6 @@ function simpletest_script_command($test_id, $test_class) {
- * @see simpletest_script_run_one_test()
- */
-function simpletest_script_cleanup($test_id, $test_class, $exitcode) {
-  if (is_subclass_of($test_class, TestCase::class)) {
-    // PHPUnit test, move on.
-    return;
-  }

I'm not 100% sure this is simpletest-specific despite the name - won't it also clean up child sites for phpunit functional tests?

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

BTB extends TestCase:

abstract class BrowserTestBase extends TestCase {

In fact all PHPUnit tests extend TestCase, so that if quoted in #18 will always be true, hence the whole thing can be refactored away.

PHPUnit functional tests do their cleanup in BrowserTestBase::cleanupEnvironment().

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

#18 same pitfall of my reasoning in #15.4. But we are removing the code that removes the artifacts, so they will be staying forever... which is fine since anyway the PHPUnit ones are not touched by the script.

xpost with #19

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

BTW we badly miss tests here, so better remove dubious code if we don't want to add some... I tried to start something in the past #3075608: Introduce TestRun objects and refactor TestDatabase to be testable but that never happened.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Doh, makes sense.

Committed fece49c and pushed to 10.0.x. Thanks!

  • catch committed fece49c on 10.0.x 
    Issue #3254726 by longwave, mondrake: Remove SimpleTest support from run...

  • catch committed 22bfdaa on 10.0.x 
    Issue #3254726 follow-up by catch: adjust phpstan baseline.
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

This missed a phpstan baseline entry, I've done that locally and pushed:

--- a/core/phpstan-baseline.neon
+++ b/core/phpstan-baseline.neon
@@ -180,11 +180,6 @@ parameters:
      count: 1
      path: lib/Drupal/Core/Template/TwigPhpStorageCache.php
 
-   -
-     message: "#^Class Drupal\\\\simpletest\\\\TestBase not found\\.$#"
-     count: 1
-     path: lib/Drupal/Core/Test/RunTests/TestFileParser.php
-
    -
      message: "#^Call to deprecated constant REQUEST_TIME\\: Deprecated in drupal\\:8\\.3\\.0 and is removed from drupal\\:10\\.0\\.0\\. Use \\\\Drupal\\:\\:time\\(\\)\\-\\>getRequestTime\\(\\); $#"
      count: 2
~
Mile23’s picture

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

❤️

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue summary: View changes
Issue tags: +10.0.0 release notes

This should probably go in the release notes. Adding a release note draft.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue summary: View changes

Status: Fixed » Closed (fixed)

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