Finish unit tests for PluginBag and subclasses

Haven't given this an in depth review but things look good and more tests are always good. Especially when the expose bugs!

1. +++ b/core/lib/Drupal/Component/Plugin/DefaultPluginBag.php
   @@ -131,6 +130,15 @@ public function getConfiguration() {
   +    // Ensure the new order matches the original order.
   +    $this->instanceIDs = $this->originalOrder = array_intersect_assoc($this->originalOrder, $this->instanceIDs);
   

   Seems like this change should just live on the PluginBag class?

2. +++ b/core/tests/Drupal/Tests/Component/Plugin/ConfigurablePluginBagTest.php
   @@ -0,0 +1,97 @@
   +  public static function getInfo() {
   

   We give these docblocks now iirc?

3. +++ b/core/tests/Drupal/Tests/Component/Plugin/ConfigurablePluginBagTest.php
   @@ -0,0 +1,97 @@
   +    $this->setupPluginBag($this->exactly(3));
   ...
   +    $this->setupPluginBag($this->exactly(3));
   

   It would be nice to assert more about what parameters are passed here but I guess that gets tricky as you are passing this to the base class.

4. +++ b/core/tests/Drupal/Tests/Component/Plugin/ConfigurablePluginBagTest.php
   @@ -0,0 +1,97 @@
   +    $expected = array(
   ...
   +    $expected = array(
   

   Can these both just share the expected data as a class property?

5. +++ b/core/tests/Drupal/Tests/Component/Plugin/ConfigurablePluginBagTest.php
   @@ -0,0 +1,97 @@
   +class TestConfigurablePlugin extends PluginBase implements ConfigurablePluginInterface {
   +  protected $configuration = array();
   

   docblocks

6. +++ b/core/tests/Drupal/Tests/Component/Plugin/DefaultPluginBagTest.php
   @@ -215,4 +123,93 @@ public function testGetConfiguration() {
   +    $expected = array('banana', 'apple');
   +    $config = $this->defaultPluginBag->getConfiguration();
   +    $this->assertSame($expected, array_keys($config), 'After removing an instance, the configuration is updated.');
   

   mh, can we test with assertArrayNotHasKey() or something here?

7. +++ b/core/tests/Drupal/Tests/Component/Plugin/DefaultSinglePluginBagTest.php
   @@ -0,0 +1,56 @@
   +  public static function getInfo() {
   

   docblock

8. +++ b/core/tests/Drupal/Tests/Component/Plugin/DefaultSinglePluginBagTest.php
   @@ -0,0 +1,56 @@
   +  protected function setupPluginBag($create_count = NULL) {
   

   docblock

9. +++ b/core/tests/Drupal/Tests/Component/Plugin/DefaultSinglePluginBagTest.php
   @@ -0,0 +1,56 @@
   +   * Tests the getConfiguration() method with configurable plugins.
   ...
   +  public function testConfigurableGetConfiguration() {
   

   This just tests the get() method.

10. +++ b/core/tests/Drupal/Tests/Component/Plugin/PluginBagTestBase.php
    @@ -0,0 +1,138 @@
    +   * @var \PHPUnit_Framework_MockObject_MockObject|\Drupal\Component\Plugin\PluginManagerInterface
    ...
    +   * @var \PHPUnit_Framework_MockObject_MockObject|\Drupal\Component\Plugin\DefaultPluginBag
    

    Not sure if I'm right, but I usually switch these the other way round.

+++ b/core/tests/Drupal/Tests/Component/Plugin/DefaultSinglePluginBagTest.php
@@ -0,0 +1,56 @@
+ protected function setupPluginBag($create_count = NULL) {
docblock

+ better typehint.

Re #9 so this is called from the get() call?

+++ b/core/tests/Drupal/Tests/Component/Plugin/DefaultSinglePluginBagTest.php
@@ -0,0 +1,56 @@
+  /**
+   * Tests the getConfiguration() method with configurable plugins.
+   */
+  public function testConfigurableGetConfiguration() {
+    $this->setupPluginBag($this->once());
+    $plugin = $this->defaultPluginBag->get('apple');
+    $this->assertSame('apple', $plugin->getPluginId());
+  }

Just read the IRC backscroll, and this clearly is named with GetConfiguration (and in docblock) but only calls get() on the plugin bag, and asserts the Id of that plugin instance. This is the one I was talking about :)

Ha, no worries :) I think this looks good now.

Committed and pushed to 8.x. Thanks!