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
Settings class has some tests, but is missing some coverage.
Proposed resolution
Note: Settings class does not seem to have any has calls to procedural code which would make it difficult to unit test.
So,
just expand the test coverage.
Remaining tasks
- check other procedural calls and identify which are wrappers
- check if there is anything to mock
- check if there are any UnitTestBase tests to convert to PHPUnit tests
User interface changes
No.
API changes
TBD (no?)
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#6 | interdiff.txt | 2.48 KB | jhedstrom |
#6 | settings-phpunit-2035863-06.patch | 2.26 KB | jhedstrom |
#3 | settings-phpunit-2035863-02.patch | 1.53 KB | jhedstrom |
#2 | settings-phpunit-2035863-02.patch | 1.53 KB | jhedstrom |
#1 | Settings-coverage-more.png | 276.63 KB | YesCT |
Comments
Comment #1
YesCT CreditAttribution: YesCT commentedcoverage of Settings, example:
It does not have it's own tests, which I would guessed would be in:
core/tests/Drupal/Tests/Component/Utility/Settings.php
I'm not sure how to generate an easily cut and pasted list of tests that are providing test coverage.
I can see the list when hover, but not copy the list.
Also, since the goal is to make things unit testable, and this seems like it already is, I'm not sure if expanding the test coverage is what we want to be doing.
If so though, would we make new tests in
core/tests/Drupal/Tests/Component/Utility/Settings.php
or look at the tests already covering some of this and add some tests there?
Comment #2
jhedstromHere's a first attempt at adding unit tests specific to this component.
Comment #3
jhedstromHere's a first attempt at adding unit tests specific to the settings component.
Comment #4
dawehnerNice!
Maybe a small assertion message would help.
Aside from that you could also test getAll()
Comment #5
dawehnerComment #6
jhedstromI added messages, and moved each method into a separate test. This has 100% coverage of Settings class.
Comment #7
dawehnerNice! Thank you very much.
Comment #8
catchCommitted/pushed to 8.x, thanks!
Comment #10
jhedstrom