Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Updated: Comment #0
Problem/Motivation
We have rudimentary tests for plugin bags, but we should finish them.
Proposed resolution
Expand the unit tests for plugin bags until 100% coverage.
Fix any bugs found along the way.
Remaining tasks
N/A
User interface changes
N/A
API changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#13 | plugin-2137947-13.patch | 22.55 KB | tim.plunkett |
#13 | interdiff.txt | 1.68 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettThis finishes the test coverage, we now have 100%.
While working on this I found a bug in DefaultPluginBag surrounding setInstanceIds(), and PluginBag's set() method. I've fixed those so that the tests pass.
Because we have two subclasses of PluginBag, and also ones dealing with ConfigurablePluginInterface, we now have 3 test classes and base class.
Comment #3
tim.plunkettInstead of updating the old test, let's remove it.
Comment #4
neclimdulHaven't given this an in depth review but things look good and more tests are always good. Especially when the expose bugs!
Comment #5
damiankloip CreditAttribution: damiankloip commentedSeems like this change should just live on the PluginBag class?
We give these docblocks now iirc?
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.
Can these both just share the expected data as a class property?
docblocks
mh, can we test with assertArrayNotHasKey() or something here?
docblock
docblock
This just tests the get() method.
Not sure if I'm right, but I usually switch these the other way round.
Comment #6
dawehner+ better typehint.
Comment #7
tim.plunkett1) No, both getConfiguration() $this->originalOrder are specific to this subclass.
2) Yep
3) Not sure about this.
4) Updated (they are different, but we can use the initial config and massage it.
5) Turns out its already declared on PluginBase
6) Sure.
7) Done
8) Done
9) Um, no it tests the ConfigurablePluginInterface specific code of getConfiguration
10) Fixed
Comment #8
damiankloip CreditAttribution: damiankloip commentedRe #9 so this is called from the get() call?
Comment #10
tim.plunkett7: plugin-2137947-7.patch queued for re-testing.
Comment #12
damiankloip CreditAttribution: damiankloip commentedJust 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 :)
Comment #13
tim.plunkettOh, there are two testConfigurableGetConfiguration(), one of which is correct. I just copied this around and forgot to rename. Sorry for the confusion.
Comment #14
damiankloip CreditAttribution: damiankloip commentedHa, no worries :) I think this looks good now.
Comment #15
webchickCommitted and pushed to 8.x. Thanks!