In light of #2863704: ConfigDiffer doesn't catch some differences, it would probably be a good idea to add more unit tests to this module.
That issue added a unit test for ConfigDiffer::same(). But it would be good to add tests for the other public methods on the base module's classes:
- [DONE] ConfigDiffer::diff()
- [DONE] ConfigLister::listTypes()
- [DONE] ConfigLister::getType()
- [DONE] ConfigLister::getTypeByPrefix()
- [DONE] ConfigLister::getTypeNameByConfigName()
- [DONE] ConfigLister::listConfig()
(Note that ConfigListerWithProviders extends ConfigLister, only adding new methods not overriding any, so they could be tested together if that is easier.)
- [DONE] ConfigListerWithProviders::listProviders()
- [DONE] ConfigListerWithProviders::getConfigProvider()
- [DONE] ConfigListerWithProviders::providerHasConfig()
- [DONE] ConfigReverter::import()
- [DONE] ConfigReverter::revert()
- [DONE] ConfigReverter::delete()
- [DONE] ConfigReverter::getFromActive()
- [DONE] ConfigReverter::getFromExtension()
We do have some coverage of many of these methods, indirectly in the ConfigUI tests, but unit test coverage would be a good idea anyway. Aside from the ConfigDiffer, however, the tests will probably not be all that easy to create. ConfigReverter and ConfigLister act on config storage, so they'll require some serious mocking/stubs. See
https://phpunit.de/manual/4.8/en/test-doubles.html
for documentation on how to do that. Also the ConfigDiffTest class has a mock already for the TranslationInterface that mocks the t() function, as one simple example.
If no one else takes this on, I will probably get to it in a few months, but probably not before end of May.
| Comment | File | Size | Author |
|---|---|---|---|
| #27 | 2863853-reverter-27.patch | 9.85 KB | jhodgdon |
| #24 | 2863853-import-test-23.patch | 12.1 KB | jhodgdon |
| #21 | 2863853-reverter-tests-21.patch | 14.13 KB | jhodgdon |
| #16 | 2863853-refactor-16.patch | 20.65 KB | jhodgdon |
| #11 | 2863853-11.patch | 9.46 KB | jhodgdon |
Comments
Comment #2
mtodor commentedHere is proposal for
diff()function testing.Basically -> one single config will be checked with multiple possible diff operations (delete, add, change, copy) -> and it will be validated that it's correct.
Comment #3
jhodgdonThis is very nice, thanks! The test passes too, always good. :)
I'm in the middle of working on another issue in this project at the moment, but when I'm done with that and my git repo is clean again, I will commit this. Updating the issue summary now.
Comment #5
jhodgdonSince the patch above has been committed, but there is more to do here, I'll set the status back to Active.
Comment #7
jhodgdonOver on #2917165: Wrong export link for migration_group I added some tests for ConfigLister methods, so updating summary here.
Comment #8
jhodgdonComment #9
jhodgdonHere is a patch that adds unit tests for the remaining methods in ConfigLister and ConfigListerWithProviders, and expands the existing tests there for the listConfig() method, to include testing for extensions. Test passes locally...
Comment #11
jhodgdonOops, I guess you can have only one @covers line in a test class, and there were some coding standards problems... take 2!
Comment #12
jhodgdonGood, it passed this time. I fixed the remaining coding standards messages for that run, and committed. Updating summary and setting back to Active.
Comment #13
jhodgdonAdded new issue component for tests...
Comment #14
jhodgdonFor some reason the last commit didn't show up here... here it is:
https://cgit.drupalcode.org/config_update/commit/?id=dd67747
Comment #15
jhodgdonI was going to add tests for ConfigReverter, and realized I would need a bunch of mocks that were also used in the tests for ConfigLister. So, I refactored the existing unit tests to use a base class. Here's patch; tests passed locally but I'll run them here before committing.
Comment #16
jhodgdonTake 2 on refactor: fix some coding standards messages and (oops) make the test base class abstract.
Comment #17
jhodgdonOK, that version worked better, committing. I'll be working on tests for ConfigReverter sometime soon.
Comment #19
jhodgdonHere are some tests for two methods on ConfigReverter. I also realized that the class didn't specify what should happen in getFromActive() if the config didn't exist, so I made it more consistent with other methods.
Comment #21
jhodgdonFixed up coding standards and gotchas...
Comment #22
jhodgdonThat's better. Committing this patch and updating issue summary. 3 methods on ConfigReverter to go, and the mock objects are getting richer...
Comment #24
jhodgdonHere's a patch that adds a test for the ConfigReverter::import() method.
Comment #26
jhodgdonGood, that passed, with a few very minor coding standards fixes, so I committed it. 2 more methods to go!
Comment #27
jhodgdonHere's a patch for the final 2 methods on the reverter...
Comment #29
jhodgdonOK, this passed, with 2 minor coding standards problems, which I fixed and committed. This is done!