Add PHPUnit tests for Config class

.

Tagging

Hi, 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

Patch needs to be updated.

Reroll of #12.

This test is very comprehensive and crafted with much thought. That is great, thanks.

1. +++ b/core/lib/Drupal/Core/Config/Config.php
   @@ -241,6 +241,8 @@ public function delete() {
   +    $this->settingsOverrides = array();
   +    $this->moduleOverrides = array();
   

   This change is wrong, config overrides are expected to persist.

2. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,474 @@
   +/**
   + * Tests the Config.
   + *
   + * @group Drupal
   + * @group Config
   + *
   + * @see \Drupal\Core\Config\Config
   + */
   

   Add a @coversDefaultClass here.

3. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,474 @@
   +   * @var \Symfony\Component\EventDispatcher\EventDispatcherInterface
   ...
   +   * @var \Drupal\Core\Config\TypedConfigManagerInterface
   

   Also specify \PHPUnit_Framework_MockObject_MockObject, e.g. @var \Drupal\Core\Config\StorageInterface|\PHPUnit_Framework_MockObject_MockObject.

4. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,474 @@
   +  /**
   +   * Test Info.
   +   */
   +  public static function getInfo() {
   +    return array(
   +      'name' => 'Config test',
   +      'description' => 'Tests Config.',
   +      'group' => 'Configuration'
   +    );
   +  }
   +
   

   Remove this, it is not necessary anymore since #697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit).

5. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,474 @@
   +  /**
   +   * Check that the config name is set correctly.
   +   *
   +   * @dataProvider setNameProvider
   +   */
   

   Function summary line should start with a third person verb. I.e. "Checks".

   Also use @covers annotation (@covers ::setName.

6. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,474 @@
   +        // Valid name with dot.
   +        'test.name',
   

   Can we test more variants of valid names here?

7. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,474 @@
   +    // Check $apply_overrides defaults to TRUE.
   +    $this->assertOriginalConfigDataEquals($settingData);
   

   I'd rather remove the default value and make the $apply_overrides parameter always required.

8. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,474 @@
   +   * @expectedException PHPUnit_Framework_Error_Warning
   

   I wouldn't expect a PHPUnit exception but rather a Config exception. What exactly is happening here?

9. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,474 @@
   +      // Check that values are cleared.
   +      $this->config->set($key, $value);
   +      if (is_array($value)) {
   ...
   +      else {
   

   My gut feeling is that this is too much logic in a test case. Can we split off the test for clearing nested values?

10. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
    @@ -0,0 +1,474 @@
    +    // Check that overrides have been reset.
    +    foreach ($moduleData as $key => $value) {
    +      $this->assertEmpty($this->config->get($key));
    +      $this->assertEmpty($this->config->getOriginal($key, TRUE));
    +    }
    

    Noted above. This is not correct, overrides should persist.

11. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
    @@ -0,0 +1,474 @@
    +  public function validateNameProvider() {
    

    I appreciate the thoroughness here.

12. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
    @@ -0,0 +1,474 @@
    +      if (is_array($value)) {
    ...
    +      else {
    

    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.

1. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,498 @@
   + * @group Drupal
   

   It has been pointed out to me in another issue that @group Drupal should not be added anymore.

2. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,498 @@
   +        'test.' . str_repeat('a', Config::MAX_NAME_LENGTH - 5),
   

   Awesome.

3. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,498 @@
   +   * @covers ::setData
   +   * @covers ::setModuleOverride
   +   * @covers ::setSettingsOverride
   +   * @covers ::save
   +   * @covers ::get
   +   * @covers ::getOriginal
   

   Use the @covers only for methods you intend to test. In this case it probably would be setModuleOverride and setSettingsOverride. This will render code coverage reports more honest. Methods which are only invoked in unrelated tests will then shine up red.

4. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,498 @@
   +  /**
   +   * Asserts all original config data equals $data provided.
   +   */
   +  public function assertOriginalConfigDataEquals($data, $apply_overrides) {
   +    foreach ($data as $key => $value) {
   +      if (is_null($apply_overrides)) {
   +        $configValue = $this->config->getOriginal($key);
   +      }
   

   I'm still a bit confused about that. At various places the method is invoked with TRUE, FALSE and NULL for the parameter $apply_overrides. Yet there is a conditional check is_null($apply_overrides) which will return FALSE if $apply_overrides is set to boolean FALSE.

   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.

Refactored to make it a bit clearer what's going on. The aim is to check that $apply_overrides defaults to TRUE - \Drupal\Core\Config\Config::getOriginal($key = '', $apply_overrides = TRUE).

Oh, I see. Would it be possible to extract this check into a separate test method (with @covers ::getOriginal bonus) and remove it from assertOriginalConfigDataEquals?

An other idea would be to eliminate assertOriginalConfigDataEquals completely. Inlining it only produces three lines of code, comparable to the following fragment in testSave():

+++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
@@ -0,0 +1,504 @@
+    // Check that original data has not been set yet.
+    foreach ($data as $key => $value) {
+      $this->assertNull($this->config->getOriginal($key, FALSE));
+    }

I've kept and simplified assertOriginalConfigDataEquals() to just check values using $apply_overrides value as it's called 8 times.

Fair enough. The assert-methods now are crystal clear and easy to read. Thanks.

I have introduced an extra check in testOverrideData() to check that $apply_overrides defaults to TRUE.

Perfect. You may choose to add back @covers ::getOriginal, now that testOverrideData() explicitly tests for that.

Going through the whole patch again, this time with Coder Sniffer in order to check conformance to Coding standards.

1. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,499 @@
   +  protected $event_dispatcher;
   ...
   +  protected $typed_config;
   

   Class property should use lower camel case, i.e. $eventDispatcher, $typedConfig.

2. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,499 @@
   +  /**
   +   * Setup.
   +   */
   

   setUp does not need documentation.

3. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,499 @@
   +     // Set the name.
   

   Indentation is wrong here, should be 4 spaces not 5.

4. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,499 @@
   +  public function testOverrideData($data, $moduleData, $settingData) {
   

   Function parameters should not use camel case, use lowercase and underscore instead, i.e. $module_data, $setting_data.

5. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,499 @@
   +    // setting data should be overriding module data.
   

   Inline comments must start with a capital letter.

6. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,499 @@
   +      $configValue = $this->config->getOriginal($key);
   +      $this->assertEquals($value, $configValue);
   

   Use lowercase and underscore for variables, i.e. $config_value.

7. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,499 @@
   +      foreach($value as $nestedKey => $nestedValue) {
   +        $fullNestedKey = $key . '.' . $nestedKey;
   +        $this->assertEquals($nestedValue, $this->config->get($fullNestedKey));
   +        $this->config->clear($fullNestedKey);
   +        $this->assertNull($this->config->get($fullNestedKey));
   

   Same here, use $nested_key, $nested_value, $full_nested_key.

8. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,499 @@
   +  public function testDelete($data, $moduleData) {
   

   Same here.

9. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,499 @@
   +  public function testMerge($data, $dataToMerge, $mergedData) {
   

   Dito.

10. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
    @@ -0,0 +1,499 @@
    +      )
    ...
    +        ))
    ...
    +          'a' => 'originalValue'
    ...
    +          'a' => 'moduleValue'
    ...
    +          'a' => 'settingValue'
    ...
    +      )
    ...
    +            'd' => 1
    ...
    +            'e' => 2
    ...
    +            'f' => 3
    +          )
    

    A comma should follow the last multiline array item.

11. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
    @@ -0,0 +1,499 @@
    +    foreach (array(':', '?', '*', '<', '>', '"',"'",'/','\\') as $char) {
    

    Add a space after every comma in the array.

12. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
    @@ -0,0 +1,499 @@
    +   * @param array $data
    ...
    +   * @param array $data
    +   * @param bool $apply_overrides
    

    @param annotations should have descriptions. Hint, reuse the ones from the class/method under test if in doubt.

13. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
    @@ -0,0 +1,499 @@
    +  }
    +}
    

    Add a newline between the closing brace of the last method and the closing brace of the class.

10 probably has too little context. Trying again:

1. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,499 @@
   +      array(
   +        // Data.
   +        array('a' => 1, 'b' => 2, 'c' => array('d' => 3)),
   +        // Data to merge.
   +        array('a' => 2, 'e' => 4, 'c' => array('f' => 5)),
   +        // Data merged.
   +        array('a' => 2, 'b' => 2, 'c' => array('d' => 3, 'f' => 5), 'e' => 4),
   +      )
   

   Comma missing on last line.

2. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,499 @@
   +        String::format('Invalid character in Config object name @name.', array(
   +          '@name' => $name,
   +        ))
   

   Comma missing on last line.

3. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,499 @@
   +        // Original data.
   +        array(
   +          'a' => 'originalValue'
   +        ),
   +        // Module overrides.
   +        array(
   +          'a' => 'moduleValue'
   +        ),
   +        // Setting overrides.
   +        array(
   +          'a' => 'settingValue'
   +        ),
   +      )
   

   Comma missing on last line, also on every line containing 'a' => '...'.

4. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,499 @@
   +          'a' => array(
   +            'd' => 1
   +          ),
   +          'b' => array(
   +            'e' => 2
   +          ),
   +          'c' => array(
   +            'f' => 3
   +          )
   

   Comma missing on last line, as well as every line containing 'X' => N.

Two final nitpiks, after that I think this is ready.

1. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,501 @@
   +   * @expectedException \Drupal\Core\Config\configValueException
   

   Class name should be uppercase here.

2. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,501 @@
   +      foreach($value as $nested_key => $nested_value) {
   

   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.

Note 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.

It 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.

1. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,487 @@
   +   * Checks that the config name is set correctly.
   

   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.

2. +++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
   @@ -0,0 +1,487 @@
   +  /**
   +   * Provides config names to test.
   +   */
   

   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.

Use \name\space\class::method() notation when referencing methods from @see annotation. See API documentation and comment standards.

@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).

+++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
@@ -0,0 +1,490 @@
+   * @see \Drupal\Tests\Core\Config::testSetName()

I think the correct namespace/class combination here is \Drupal\Tests\Core\Config\ConfigTest::testSetName()

xpost 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.

diff --git a/core/tests/Drupal/Tests/Core/Config/ConfigTest.php b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
index 8e3085e..4111e08 100644
--- a/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
+++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
@@ -76,7 +76,7 @@ public function testSetName($name) {
   /**
    * Provides config names to test.
    *
-   * @see \Drupal\Tests\Core\Config::testSetName()
+   * @see \Drupal\Tests\Core\Config\ConfigTest::testSetName()
    */
   public function setNameProvider() {
     return array(
@@ -330,7 +330,7 @@ public function testMerge($data, $data_to_merge, $merged_data) {
   /**
    * Provides data to test merges.
    *
-   * @see \Drupal\Tests\Core\Config::testMerge()
+   * @see \Drupal\Tests\Core\Config\ConfigTest::testMerge()
    */
   public function mergeDataProvider() {
     return array(
@@ -358,7 +358,7 @@ public function testValidateNameException($name, $exception_message) {
   /**
    * Provides data to test name validation.
    *
-   * @see \Drupal\Tests\Core\Config::testValidateNameException()
+   * @see \Drupal\Tests\Core\Config\ConfigTest::testValidateNameException()
    */
   public function validateNameProvider() {
     $return = array(
@@ -394,8 +394,8 @@ public function validateNameProvider() {
   /**
    * Provides override data.
    *
-   * @see \Drupal\Tests\Core\Config::testOverrideData()
-   * @see \Drupal\Tests\Core\Config::testDelete()
+   * @see \Drupal\Tests\Core\Config\ConfigTest::testOverrideData()
+   * @see \Drupal\Tests\Core\Config\ConfigTest::testDelete()
    */
   public function overrideDataProvider() {
     return array(
@@ -419,7 +419,7 @@ public function overrideDataProvider() {
   /**
    * Provides simple test data.
    *
-   * @see \Drupal\Tests\Core\Config::testClear()
+   * @see \Drupal\Tests\Core\Config\ConfigTest::testClear()
    */
   public function simpleDataProvider() {
     return array(
@@ -436,11 +436,11 @@ public function simpleDataProvider() {
   /**
    * Provides nested test data.
    *
-   * @see \Drupal\Tests\Core\Config::testSetData()
-   * @see \Drupal\Tests\Core\Config::testSave()
-   * @see \Drupal\Tests\Core\Config::testSetValue()
-   * @see \Drupal\Tests\Core\Config::testInitWithData()
-   * @see \Drupal\Tests\Core\Config::testNestedClear()
+   * @see \Drupal\Tests\Core\Config\ConfigTest::testSetData()
+   * @see \Drupal\Tests\Core\Config\ConfigTest::testSave()
+   * @see \Drupal\Tests\Core\Config\ConfigTest::testSetValue()
+   * @see \Drupal\Tests\Core\Config\ConfigTest::testInitWithData()
+   * @see \Drupal\Tests\Core\Config\ConfigTest::testNestedClear()
    */
   public function nestedDataProvider() {
     return array(

@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.