Discovered via #2175459: Convert Drupal\system\Tests\InstallerTest into a new Drupal\simpletest\InstallerTestBase

InstallerTranslationTest is supposed to fail with this patch, because it does not define any test* methods, and thus, the test is not actually executed.

Hopefully that's the only test.

#2201783: Simplify execution logic in TestBase::run() should be committed first.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

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

sun’s picture

Status: Needs work » Needs review

drupal8.test-empty.0.patch queued for re-testing.

Status: Needs review » Needs work

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

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

sun’s picture

Status: Needs work » Needs review
FileSize
2.07 KB

Now including fix.

sun’s picture

#2201783: Simplify execution logic in TestBase::run() has landed, so this is ready to go now.

tstoeckler’s picture

Status: Needs review » Needs work

It looks good, but the if(TRUE) should be removed before commit.

sun’s picture

Status: Needs work » Needs review

That no longer exists in the latest patch? ;)

tstoeckler’s picture

Sorry, don't know how that happened. Latest patch looks great. I have one minor comment, though:

+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -806,6 +806,9 @@ public function run(array $methods = array()) {
+      $this->assert(FALSE, 'No test methods found.', 'Requirements', array('function' => __METHOD__ . '()', 'file' => __FILE__, 'line' => __LINE__));

A little further down we use TestBase::insertAssert() directly. On the other hand I would have expected $this->fail() instead. Thoughts?

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Ahh, OK we use TestBase::insertAssert() so we can get the assertion ID. That is not needed here, so we should really be using $this->fail() as far as I can tell.

That's super minor, though, so let's just get this in.

sun’s picture

To clarify, TestBase::fail() does not allow to pass a custom $caller information.

That's required here, because the automated detection of $caller explicitly skips all methods in test base classes, so without supplying a custom $caller, the failing assertion would expose simpletest_batch_operation() as the originating code file/line.

tstoeckler’s picture

Oohh, that's interesting. Thanks!

Would have been great to have a comment for that as I can easily see people wanting to "fix" that in the future. But anyway, still RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Nice optimization.

Committed/pushed to 8.x, with this comment added above the assert:

      // Call $this->assert() here because we need to pass along custom caller
      // information, lest the wrong originating code file/line be identified.

Hopefully that's mostly right. :)

tstoeckler’s picture

Awesome, thanks! sun++, webchick++ :-)

Status: Fixed » Closed (fixed)

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