Problem/Motivation

In #2850797: Prepare our phpunit tests to be BC compatible with phpunit 5.x/6.x we found that some component tests are extending the core class Drupal\Tests\UnitTestCase. They should be extending PHPUnit\Framework\TestCase instead.

Proposed resolution

Replace Drupal\Tests\UnitTestCase by PHPUnit\Framework\TestCase.

Remaining tasks

Review.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#10 2866894-10.patch48.67 KBmaxocub
#7 2866894-7.patch48.63 KBmaxocub
#7 2866894-7-test-only.patch1.54 KBmaxocub
#3 2866894-3.patch47.08 KBmaxocub
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

maxocub created an issue. See original summary.

maxocub’s picture

maxocub CreditAttribution: maxocub commented
Title: Components tests should not use Drupal\Tests\UnitTestCase but PHPUnit\Framework\TestCase » Component tests should not use Drupal\Tests\UnitTestCase but PHPUnit\Framework\TestCase
Issue summary: View changes
maxocub’s picture

maxocub CreditAttribution: maxocub commented
Status: Active » Needs review
FileSize
2866894-3.patch47.08 KB

Here some additional modifications that were required:

  • Replaced a few calls to randomMachineName(), which was provided by UnitTestCase, by the name() method from the Random() class.
  • Replaced some calls to assertArrayEquals() by assertEquals().
  • Ensured that FileCacheFactory had a prefix for some discovery tests.
Mile23’s picture

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

+1 on this.

Also, we should add a test listener to make sure that tests in the Drupal\Tests\Component\ namespace don't inherit from UTC, KTB or BTB or other Drupal test bases. That will avoid regressions.

Look at DrupalStandardsListener for an example of how to do this.

maxocub’s picture

maxocub CreditAttribution: maxocub commented

@Mile23: Good idea, I'll do that, thanks.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi at jobiqo - job board technology commented
Status: Needs review » Needs work

Patch looks good, needs work for the listener.

maxocub’s picture

maxocub CreditAttribution: maxocub commented
Status: Needs work » Needs review
FileSize
2866894-7-test-only.patch1.54 KB
2866894-7.patch48.63 KB

First time using a test listener, so not sure if I'm doing it right.
The test-only patch is the interdiff from the last patch.

maxocub’s picture

maxocub CreditAttribution: maxocub commented

There's still 3 core classes that are used in a component test:

core/tests/Drupal/Tests/Component/Plugin/DefaultFactoryTest.php:8:use Drupal\plugin_test\Plugin\plugin_test\fruit\Cherry;
core/tests/Drupal/Tests/Component/Plugin/DefaultFactoryTest.php:9:use Drupal\plugin_test\Plugin\plugin_test\fruit\FruitInterface;
core/tests/Drupal/Tests/Component/Plugin/DefaultFactoryTest.php:10:use Drupal\plugin_test\Plugin\plugin_test\fruit\Kale;

I guess we should open a follow-up since it's out of scope of this issue?

The last submitted patch, 7: 2866894-7-test-only.patch, failed testing.

maxocub’s picture

maxocub CreditAttribution: maxocub commented
FileSize
2866894-10.patch48.67 KB

Patch no longer applied, Re-rolled.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Listener-only patch in #7 proves that the listener works.

Everything else looks good.

+++ b/core/tests/Drupal/Tests/Component/Plugin/Discovery/StaticDiscoveryDecoratorTest.php
@@ -220,7 +220,7 @@ function () {
-    $this->assertArrayEquals(
+    $this->assertEquals(

I was a little concerned about the instances of this substitution but the tests pass, so yay. assertArrayEquals() applies ksort() to both arrays before comparing. But the tests pass here, and will fail if the arrays are assembled differently in the future. This is better than modifying the arrays so they're comparable because the expectation is more exact.

  • larowlan committed 13e93c0 on 8.5.x 
    Issue #2866894 by maxocub: Component tests should not use Drupal\Tests\...
larowlan’s picture

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

This is better than modifying the arrays so they're comparable because the expectation is more exact.

Agree

Committed as 13e93c0 and pushed to 8.5.x.

Cherry-picked as 00c6cf4 and pushed to 8.4.x.

Status: Fixed » Closed (fixed)

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

maxocub’s picture

maxocub CreditAttribution: maxocub as a volunteer and commented