Problem/Motivation

While doing some analysis for #2364555: Add @covers annotation, fix some --strict for PHPUnit, I found that the unit tests for Drupal\Core\Plugin\DefaultPluginManager go to great length to unit test ::createInstance(), a method which DefaultPluginManager doesn't actually implement. It inherits this method from PluginManagerBase.

Combined with #2052109: [meta] Expand phpunit tests for \Drupal\Component\Plugin classes, I thought I'd do a more pure mocked unit test of PluginManagerBase.

Proposed resolution

Evaluate the patch, commit the changes, feel all warm and fuzzy.

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Unfrozen changes Unfrozen because it only improved automated testing.
CommentFileSizeAuthor
#10 interdiff.txt3.8 KBmile23
#10 2371531_10.patch5.06 KBmile23
#9 2371531.9.patch5.02 KByesct
#9 interdiff.2371531.7.9.txt2.54 KByesct
#7 interdiff.txt653 bytesmile23
#7 2371531_7.patch4.93 KBmile23
#5 interdiff.txt533 bytesmile23
#5 2371531_5.patch5.1 KBmile23
#2 2371531_1.patch5.09 KBmile23

Comments

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Issue summary: View changes
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
StatusFileSize
new 2371531_1.patch5.09 KB

This test only covers ::createInstance(). Other methods scored CRAP of 2, the lowest possible.

We also use a mock test class, since some code paths within ::createInstance() require that the object implement a specific interface.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: 2371531_1.patch, failed testing.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review
StatusFileSize
new 2371531_5.patch5.1 KB
new interdiff.txt533 bytes

DrupalComponentTest to the rescue. :-)

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented

2/2 tests PASS.
53% coverage.

Usual comments:

+++ b/core/tests/Drupal/Tests/Component/Plugin/PluginManagerBaseTest.php
@@ -0,0 +1,102 @@
+use Drupal\Component\Plugin\PluginManagerBase;
+use Drupal\Component\Plugin\Discovery\DiscoveryInterface;
+use Drupal\Component\Plugin\FallbackPluginManagerInterface;

Unused :)

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Issue summary: View changes
StatusFileSize
new 2371531_7.patch4.93 KB
new interdiff.txt653 bytes

Thanks!

Removed extra uses.

Also added beta evaluation thingie.

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
Status: Needs review » Reviewed & tested by the community

:)

yesct’s picture

yesct commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new interdiff.2371531.7.9.txt2.54 KB
new 2371531.9.patch5.02 KB 
  1. +++ b/core/tests/Drupal/Tests/Component/Plugin/MockFallbackPluginManager.php
@@ -0,0 +1,30 @@
+  /**
+   * {@inheritdoc}
+   *
+   * Minimally implement getFallbackPluginId so that we can test it.
+   */

    we dont allow additional docs in an inheritdoc like this. it doesn't work with out docs parser. #1994890: Allow {@inheritdoc} and additional documentation 

  2. +++ b/core/tests/Drupal/Tests/Component/Plugin/PluginManagerBaseTest.php
@@ -0,0 +1,99 @@
+ * @uses Drupal\Component\Plugin\Exception\PluginNotFoundException

    @uses?

    I dont see that in #2057905: [policy, no patch] Discuss the standards for phpunit based tests 

  3. +++ b/core/tests/Drupal/Tests/Component/Plugin/PluginManagerBaseTest.php
@@ -0,0 +1,99 @@
+   * Generate a mocked FactoryInterface object with known properties.
...
+   * Test createInstance() with no fallback methods.

    and others.
    Should be third person verbs.

    I'll just fix that. 

  4. +++ b/core/tests/Drupal/Tests/Component/Plugin/MockFallbackPluginManager.php
@@ -0,0 +1,30 @@
+use Drupal\Component\Plugin\PluginManagerBase;
+use Drupal\Component\Plugin\FallbackPluginManagerInterface;

    we are adding these use's all at once, might as well be alphabetical order. (no order already there to mess up) 

  5. +++ b/core/tests/Drupal/Tests/Component/Plugin/MockFallbackPluginManager.php
@@ -0,0 +1,30 @@
+ * Mock FallbackPluginManagerInterface for PluginManagerBaseTest.
+ *
+ * We have to mock FallbackPluginManagerInterface here so that we can implement

    supposed to use full namespaced paths in comments. (I know it makes for really long comments...)

    I reworded. See if it is still accurate.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
StatusFileSize
new 2371531_10.patch5.06 KB
new interdiff.txt3.8 KB

Thanks for the review.

Removed @uses.

Renamed MockFallbackPluginManager to StubFallbackPluginManager because it's really a stub and not a mock.

Poked some inline comments.

yesct’s picture

yesct commented

Is the code coverage more accurate or the same with the @uses ?

I asked about it in #2057905-54: [policy, no patch] Discuss the standards for phpunit based tests

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Is the code coverage more accurate or the same with the @uses ?

More.

I answered more indepth-ly over there, too.

daffie’s picture

daffie commented
Status: Needs review » Needs work

The function PluginManagerBase::createInstance has a 100% coverage according to phpunit coverage.
The does what is asked in the issue summary.
The tests test the ::createInstance function.
The documentation is in order.
The patch gets a green light from the test-server.

+++ b/core/tests/Drupal/Tests/Component/Plugin/PluginManagerBaseTest.php
@@ -0,0 +1,98 @@
-    $manager = new StubFallbackPluginManager();
+    $manager = new \Drupal\Tests\Component\Plugin\StubFallbackPluginManager();

Shouldn't we add this minor change. If not, why?

For the rest the patch is RTBC for me.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review

Whither this:

+    $manager = new \Drupal\Tests\Component\Plugin\StubFallbackPluginManager();

StubFallbackPluginManager is in the same namespace as the test class, so we don't have to qualify it.

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

@Mile23: I did not know that. Learned something new.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 2371531_10.patch, failed testing.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review

Patch in #10 still applies and passes tests locally. The tests which failed were unrelated. Asking the testbot yet again...

Mile23 queued 10: 2371531_10.patch for re-testing.

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

Before the retest failed it was RTBC. Nothing has changed and the testbot is happy again. So back to RTBC.

  • webchick committed ba40d78 on 8.0.x 
    Issue #2371531 by Mile23, YesCT, daffie: Expand unit testing for Drupal\...
daffie’s picture

daffie commented
Status: Reviewed & tested by the community » Fixed

Was committed by webchick. So setting status to fixed.

Status: Fixed » Closed (fixed)

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