Spin-off from #1411074: Add a flag to set up test environment only once per test class; introduce setUpBeforeClass() and tearDownAfterClass() [PHPUnit]

TestBase::run() contains some unnecessary layers of control structure nesting, which make it hard to change the execution flow.

Attached patch performs no functional changes, and only removes the nesting:

  1. The test class requirements check is moved first.
  2. Test methods are immediately filtered upon retrieval, instead of within the foreach loop.
CommentFileSizeAuthor
#2 drupal8.test-run.2.patch7.26 KBsun
drupal8.test-run.0.patch7.03 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, drupal8.test-run.0.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
drupal8.test-run.2.patch7.26 KB

D'oh! :)

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -791,85 +803,76 @@ public function run(array $methods = array()) {
+    if (defined("$class::SORT_METHODS")) {
+      sort($test_methods);
     }

I know this is just moved but it really makes no sense :)

Not something to worry about here, but do we really to support something like that? It's not documented or used anywhere.

It's not that visible in the patch, but if you apply it, the code in run() is much less nested and easier to follow. Looks good to me.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Yeah, I also wondered about that SORT_METHODS class constant thingie — I didn't know that this existed, and I do not really understand how that makes any sense.

Created #2202377: Remove Test::SORT_METHODS constant opt-in support for sorting test methods alphabetically

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

2: drupal8.test-run.2.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

2: drupal8.test-run.2.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Two issues are blocked on this:

#2206417: Bogus tests without test methods are reported as successful tests
#1411074: Add a flag to set up test environment only once per test class; introduce setUpBeforeClass() and tearDownAfterClass() [PHPUnit]

Eagerly waiting for this commit.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Looks good to me. Committed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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