Problem/Motivation

This only barely counts as bug.

Working on #2415441: Automate finding @covers errors, I encountered ConfigurablePluginCollectionTest which has this annotation:

@coversDefaultClass \Drupal\Component\Plugin\ConfigurablePluginInterface

Generally, you can't test an interface.

Further research led me to discover that ConfigurablePluginCollectionTest really tests DefaultLazyPluginCollection. There is also an existing DefaultLazyPluginCollectionTest, which duplicates effort quite a bit.

ConfigurablePluginCollectionTest and DefaultLazyPluginCollectionTest have nearly identical code in their tests, because they're both extending the same subclass (LazyPluginCollectionTestBase) that provides the same fixture to both.

The fixture is a DefaultLazyPluginCollection with a bunch of mocked ConfigurablePluginInterface objects.

Therefore ConfigurablePluginCollectionTest is actually exercising DefaultLazyPluginCollection, which already has a test which is why we should merge them.

Proposed resolution

  1. Merge the tests from ConfigurablePluginCollectionTest into DefaultLazyPluginCollectionTest.
  2. Improve annotation while we're there.

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Unfrozen changes Unfrozen because it only improves automated tests.
CommentFileSizeAuthor
#1 2423241_1.patch7.17 KBmile23

Comments

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Active » Needs review
StatusFileSize
new 2423241_1.patch7.17 KB

Not a lot of analysis here, just doing a straight-up merge of the two test classes. There's likely some overlap, but I'm not familiar enough with the plugin system to know if I'd be obliterating any subtleties.

daffie’s picture

daffie commented
Status: Needs review » Needs work

I am not happy with the merge. The problem for me is the class DefaultLazyPluginCollection uses the ConfigurablePluginInterface, but does not implement it.
The ConfigurablePluginCollectionTest with the class TestConfigurablePlugin is in my eyes not very useful. You are only testing test code.
The best solution is to delete the ConfigurablePluginCollectionTest and add new tests where the ConfigurablePluginInterface is implemented.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Priority: Normal » Minor
Issue summary: View changes

You can't really test an interface which is what ConfigurablePluginCollectionTest tries to tell us it's doing.

ConfigurablePluginCollectionTest and DefaultLazyPluginCollectionTest have nearly identical code in their tests, because they're both extending the same subclass (LazyPluginCollectionTestBase) that provides the same fixture to both.

The fixture is a DefaultLazyPluginCollection with a bunch of mocked ConfigurablePluginInterface objects.

Therefore ConfigurablePluginCollectionTest is actually exercising DefaultLazyPluginCollection, which already has a test which is why we should merge them. :-)

daffie’s picture

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

It all looks good to me.
The ConfigurablePluginInterface tests are moved to DefaultLazyPluginCollectionTest.
Personally the ConfigurablePluginInterface tests could have been deleted completely and not have been moved to an other location, but I can live with this solution too.
It is all about tests and documentation, so it is allowed for beta-changes.
For me it is RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed b40b527 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed b40b527 on 8.0.x 
    Issue #2423241 by Mile23: Merge ConfigurablePluginCollectionTest into...

Status: Fixed » Closed (fixed)

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