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.
Problem/Motivation
There are no PHPUnit tests for the config class
Remaining tasks
Create PHPUnit tests to check that the config class is working as expected
Comment | File | Size | Author |
---|---|---|---|
#39 | interdiff.txt | 2.62 KB | davidgrayston |
#39 | 2206571-39-phpunit-tests-config.patch | 13.68 KB | davidgrayston |
#37 | interdiff.txt | 2.19 KB | davidgrayston |
#35 | 2206571-35-phpunit-tests-config.patch | 13.57 KB | davidgrayston |
#34 | 2206571-34-phpunit-tests-config.patch | 13.57 KB | davidgrayston |
Comments
Comment #1
davidgrayston CreditAttribution: davidgrayston commentedComment #2
davidgrayston CreditAttribution: davidgrayston commentedHere's another patch with more unit tests.
$this->settingsOverrides
,$this->languageOverrides
and$this->moduleOverrides
are now also reset in\Drupal\Core\Config\Config::delete()
.I'm not sure if this is the correct fix, but after a Config->delete(), Config->get() was still returning override values.
Comment #3
davidgrayston CreditAttribution: davidgrayston commentedFixed whitespace
Comment #4
dawehner.
Comment #5
davidgrayston CreditAttribution: davidgrayston commentedTo reproduce issue where override values are not reset on delete
Value of "a" is still "overrideVal"
$this->config->get('a');
Comment #6
davidgrayston CreditAttribution: davidgrayston commentedtestDelete() only passes if the override values are cleared in \Drupal\Core\Config\Config::delete()
Comment #7
alexpottTagging
Comment #8
davidgrayston CreditAttribution: davidgrayston commentedUpdated patch for latest 8.x
Comment #9
davidgrayston CreditAttribution: davidgrayston commentedUpdated patch for latest 8.x
Comment #10
ParisLiakos CreditAttribution: ParisLiakos commentedHi, thanks for the tests!
I noticed though that many of your tests could use the help of data providers:
http://phpunit.de/manual/3.7/en/writing-tests-for-phpunit.html#writing-t...
also your test file misses the @file doc
Comment #11
davidgrayston CreditAttribution: davidgrayston commentedThanks, I've used a provider where different conditions needed to be checked e.g. testValidateNameException() - I'll review the more basic tests to see if a provider is necessary
Thanks for taking a look
Comment #12
davidgrayston CreditAttribution: davidgrayston commentedUpdated tests to use data providers
Comment #13
Jalandhar CreditAttribution: Jalandhar commentedPatch needs to be updated.
Comment #14
amitgoyal CreditAttribution: amitgoyal commentedReroll of #12.
Comment #15
znerol CreditAttribution: znerol commentedThis test is very comprehensive and crafted with much thought. That is great, thanks.
This change is wrong, config overrides are expected to persist.
Add a
@coversDefaultClass
here.Also specify
\PHPUnit_Framework_MockObject_MockObject
, e.g.@var \Drupal\Core\Config\StorageInterface|\PHPUnit_Framework_MockObject_MockObject
.Remove this, it is not necessary anymore since #697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit).
Function summary line should start with a third person verb. I.e. "Checks".
Also use
@covers
annotation (@covers ::setName.Can we test more variants of valid names here?
I'd rather remove the default value and make the
$apply_overrides
parameter always required.I wouldn't expect a PHPUnit exception but rather a Config exception. What exactly is happening here?
My gut feeling is that this is too much logic in a test case. Can we split off the test for clearing nested values?
Noted above. This is not correct, overrides should persist.
I appreciate the thoroughness here.
I am rather unsure about that but I think that
$config->get
will return an array for nested objects. If this is true, then it is not necessary to differentiate between array and scalar values.Comment #16
davidgrayston CreditAttribution: davidgrayston commentedI've made suggested changes:
Comment #17
davidgrayston CreditAttribution: davidgrayston commentedComment #18
znerol CreditAttribution: znerol commentedIt has been pointed out to me in another issue that
@group Drupal
should not be added anymore.Awesome.
Use the
@covers
only for methods you intend to test. In this case it probably would besetModuleOverride
andsetSettingsOverride
. This will render code coverage reports more honest. Methods which are only invoked in unrelated tests will then shine up red.I'm still a bit confused about that. At various places the method is invoked with
TRUE
,FALSE
andNULL
for the parameter$apply_overrides
. Yet there is a conditional checkis_null($apply_overrides)
which will returnFALSE
if$apply_overrides
is set to booleanFALSE
.I think it is easier to understand if the
is_null
check is removed and instead$apply_overrides
is always set to a boolean.Also it will help readability if @param and @return annotations here are added.
Comment #19
davidgrayston CreditAttribution: davidgrayston commented$apply_overrides
defaults to TRUE -\Drupal\Core\Config\Config::getOriginal($key = '', $apply_overrides = TRUE)
. I've also added @param to methods where necessary. None of them return anything, so @return has been omitted.Comment #20
davidgrayston CreditAttribution: davidgrayston commentedPatch updated with correction to comment
Comment #21
znerol CreditAttribution: znerol commentedOh, I see. Would it be possible to extract this check into a separate test method (with
@covers ::getOriginal
bonus) and remove it fromassertOriginalConfigDataEquals
?An other idea would be to eliminate
assertOriginalConfigDataEquals
completely. Inlining it only produces three lines of code, comparable to the following fragment intestSave()
:Comment #22
davidgrayston CreditAttribution: davidgrayston commentedI've kept and simplified
assertOriginalConfigDataEquals()
to just check values using$apply_overrides
value as it's called 8 times.I have introduced an extra check in
testOverrideData()
to check that$apply_overrides
defaults to TRUE.Comment #23
znerol CreditAttribution: znerol commentedFair enough. The assert-methods now are crystal clear and easy to read. Thanks.
Perfect. You may choose to add back
@covers ::getOriginal
, now thattestOverrideData()
explicitly tests for that.Going through the whole patch again, this time with Coder Sniffer in order to check conformance to Coding standards.
Class property should use lower camel case, i.e.
$eventDispatcher
,$typedConfig
.setUp
does not need documentation.Indentation is wrong here, should be 4 spaces not 5.
Function parameters should not use camel case, use lowercase and underscore instead, i.e.
$module_data
,$setting_data
.Inline comments must start with a capital letter.
Use lowercase and underscore for variables, i.e.
$config_value
.Same here, use
$nested_key
,$nested_value
,$full_nested_key
.Same here.
Dito.
A comma should follow the last multiline array item.
Add a space after every comma in the array.
@param
annotations should have descriptions. Hint, reuse the ones from the class/method under test if in doubt.Add a newline between the closing brace of the last method and the closing brace of the class.
Comment #24
znerol CreditAttribution: znerol commented10 probably has too little context. Trying again:
Comma missing on last line.
Comma missing on last line.
Comma missing on last line, also on every line containing
'a' => '...'
.Comma missing on last line, as well as every line containing
'X' => N
.Comment #25
davidgrayston CreditAttribution: davidgrayston commentedPatch rerolled with coding standards
Comment #26
znerol CreditAttribution: znerol commentedTwo final nitpiks, after that I think this is ready.
Class name should be uppercase here.
Insert a space between the
foreach
keyword and the opening brace.I'm still unsure about the
@expectedException PHPUnit_Framework_Error_Warning
but let's leave that decision to core comitters.Comment #27
davidgrayston CreditAttribution: davidgrayston commentedThanks, I've fixed the typo/formatting and also attached a version of the patch without the warning test.
Perhaps the Config class needs to be changed to throw an exception in that situation.
Comment #28
znerol CreditAttribution: znerol commentedNote to core committers: The difference between the two patches in #27 is that the
no-warning.patch
does not contain one test which uses an@expectedException PHPUnit_Framework_Error_Warning
annotation (see attached interdiff).Like pointed out by @davidgrayston it would also be possible to refactor
Config::set()
such that it throws a proper exception if something tries to set a nested key on a scalar value. Currently a PHP warning is issued in that case.Comment #29
alexpottIt is great to see this patch nearly done. I think we should create a new issue to handle the php warning created by treating a scalar value as an array - nice find btw. I think it is good to leave the test in since it encoding the current behaviour.
It is no longer necessary to have test method descriptions - the method name and @covers should be enough - along with comments in the test (of which you have plenty - nice work). Please remove from all the test methods - this will fix at least one or two minor nits to do with line length and grammar.
Data providers still need to documenting - core is yet to truly coalesce to any form of standard what what is helpful is when the method is it providing is linked. I think we should add
@see testSetName()
since it is concise.Comment #30
davidgrayston CreditAttribution: davidgrayston commentedThank you, I've updated the patch with test method descriptions removed and @see added to provider docs
Comment #31
davidgrayston CreditAttribution: davidgrayston commentedComment #32
znerol CreditAttribution: znerol commentedUse
\name\space\class::method()
notation when referencing methods from@see
annotation. See API documentation and comment standards.Comment #33
davidgrayston CreditAttribution: davidgrayston commentedThanks – patch updated with correction to @see docs
Comment #34
davidgrayston CreditAttribution: davidgrayston commentedThat was slightly wrong - here's an updated patch
Comment #35
davidgrayston CreditAttribution: davidgrayston commentedSorry, again that was wrong, here's an updated patch
Comment #36
alexpott@davidgrayston it's really helpful if in future you could provide interdiffs so it's super simple to see what has changed in each patch (yes drupal.org could help here).
Comment #37
davidgrayston CreditAttribution: davidgrayston commentedSkipping out the mistakes, here's an interdiff between these patches:
2206571-30-phpunit-tests-config.patch
2206571-35-phpunit-tests-config.patch
Comment #38
znerol CreditAttribution: znerol commentedI think the correct namespace/class combination here is
\Drupal\Tests\Core\Config\ConfigTest::testSetName()
Comment #39
davidgrayston CreditAttribution: davidgrayston commentedYes, you are right, here's the corrected patch
Comment #40
alexpottxpost with #39.
This issue is a unfrozen change (additional test coverage) as per #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase? and it's benefits outweigh any disruption. Committed d229eed and pushed to 8.0.x. Thanks!
re #38 yep you are correct - fixing on commit.
Comment #43
alexpott@davidgrayston it'd be awesome if you could open a new issue to address the bug you found when by treating a scalar value as an array.
Comment #44
davidgrayston CreditAttribution: davidgrayston commentedThanks! I've created the new issue here #2369613: Warning when treating scalars as arrays in Config::set()