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

Files: 
CommentFileSizeAuthor
#39 interdiff.txt2.62 KBdavidgrayston
#39 2206571-39-phpunit-tests-config.patch13.68 KBdavidgrayston
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#37 interdiff.txt2.19 KBdavidgrayston
#35 2206571-35-phpunit-tests-config.patch13.57 KBdavidgrayston
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,961 pass(es). View
#34 2206571-34-phpunit-tests-config.patch13.57 KBdavidgrayston
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,956 pass(es). View
#33 2206571-33-phpunit-tests-config.patch13.56 KBdavidgrayston
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,951 pass(es). View
#30 2206571-30-phpunit-tests-config.patch13.26 KBdavidgrayston
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,957 pass(es). View
#28 no-warning-interdiff.txt837 bytesznerol
#28 no-warning-interdiff.txt837 bytesznerol
#27 2206571-27-phpunit-tests-config-no-warning.patch13.35 KBdavidgrayston
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,992 pass(es). View
#27 2206571-27-phpunit-tests-config.patch13.75 KBdavidgrayston
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,833 pass(es). View
#25 2206571-25-phpunit-tests-config.patch13.75 KBdavidgrayston
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,833 pass(es). View
#22 2206571-22-phpunit-tests-config.patch13.58 KBdavidgrayston
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,878 pass(es). View
#20 2206571-20-phpunit-tests-config.patch13.8 KBdavidgrayston
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,739 pass(es). View
#19 2206571-19-phpunit-tests-config.patch13.81 KBdavidgrayston
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,876 pass(es). View
#16 2206571-16-phpunit-tests-config.patch13.53 KBdavidgrayston
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,711 pass(es). View
#14 2206571-14-phpunit-tests-config.patch13.76 KBamitgoyal
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,347 pass(es). View
#12 2206571-12-phpunit-tests-config.patch14.34 KBdavidgrayston
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,245 pass(es). View
#9 2206571-9-phpunit-tests-config.patch11.12 KBdavidgrayston
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,710 pass(es). View
#8 2206571-8-phpunit-tests-config.patch11.96 KBdavidgrayston
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,592 pass(es). View
#3 2206571-3-phpunit-tests-config.patch13.21 KBdavidgrayston
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,823 pass(es). View
#2 2206571-2-phpunit-tests-config.patch13.21 KBdavidgrayston
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,855 pass(es). View
#1 2206571-phpunit-tests-config.patch5.87 KBdavidgrayston
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,844 pass(es). View

Comments

davidgrayston’s picture

FileSize
5.87 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,844 pass(es). View
davidgrayston’s picture

FileSize
13.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,855 pass(es). View

Here'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.

davidgrayston’s picture

FileSize
13.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,823 pass(es). View

Fixed whitespace

dawehner’s picture

Status: Active » Needs review

.

davidgrayston’s picture

To reproduce issue where override values are not reset on delete

$this->config->set('a', 'testVal');
$this->config->setModuleOverride(array('a' => 'overrideVal'));
$this->config->save();
$this->config->delete();

Value of "a" is still "overrideVal"
$this->config->get('a');

davidgrayston’s picture

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

+++ b/core/tests/Drupal/Tests/Core/Config/ConfigTest.php
@@ -0,0 +1,337 @@
+  public function testDelete() {
...
+    $this->assertEmpty($this->config->get('a'));
+    $this->assertEmpty($this->config->getOriginal('a', TRUE));

testDelete() only passes if the override values are cleared in \Drupal\Core\Config\Config::delete()

alexpott’s picture

Issue tags: +Configuration system

Tagging

davidgrayston’s picture

FileSize
11.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,592 pass(es). View

Updated patch for latest 8.x

davidgrayston’s picture

FileSize
11.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,710 pass(es). View

Updated patch for latest 8.x

ParisLiakos’s picture

Status: Needs review » Needs work

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

davidgrayston’s picture

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

davidgrayston’s picture

Status: Needs work » Needs review
FileSize
14.34 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,245 pass(es). View

Updated tests to use data providers

Jalandhar’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch needs to be updated.

amitgoyal’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
13.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,347 pass(es). View

Reroll of #12.

znerol’s picture

Status: Needs review » Needs work

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.

davidgrayston’s picture

FileSize
13.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,711 pass(es). View

I've made suggested changes:

  1. Removed unnecessary fix
  2. Added @coversDefaultClass
  3. Specified mock objects
  4. Removed getInfo()
  5. Added @covers + corrected docs
  6. Added testing of maximum length too
  7. $apply_overrides is now always required
  8. The test can be updated to expect a Config exception if/when the Config class is refactored to validate and throw an exception – it currently generates a warning. Alternatively we can remove this test? - I’ve also added an extra test to check the validation in ::set, which does throw ConfigValueException
  9. Tests have been split
  10. Updated tests to check overrides persist
  11. Cheers
  12. I’ve simplified this to just compare the complete value. The idea was to check that directly accessing nested values worked e.g. my.nested.value, but this might be overkill.
davidgrayston’s picture

Status: Needs work » Needs review
znerol’s picture

  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.

davidgrayston’s picture

FileSize
13.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,876 pass(es). View
  1. Removed
  2. Thanks
  3. Removed methods that aren't directly related (I was in 2 minds about ::getOriginal)
  4. 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). I've also added @param to methods where necessary. None of them return anything, so @return has been omitted.
davidgrayston’s picture

FileSize
13.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,739 pass(es). View

Patch updated with correction to comment

znerol’s picture

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));
+    }
davidgrayston’s picture

FileSize
13.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,878 pass(es). View

I'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.

znerol’s picture

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.

znerol’s picture

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.

davidgrayston’s picture

FileSize
13.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,833 pass(es). View

Patch rerolled with coding standards

znerol’s picture

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.

davidgrayston’s picture

FileSize
13.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,833 pass(es). View
13.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,992 pass(es). View

Thanks, 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.

znerol’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
837 bytes
837 bytes

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

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.

davidgrayston’s picture

FileSize
13.26 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,957 pass(es). View

Thank you, I've updated the patch with test method descriptions removed and @see added to provider docs

davidgrayston’s picture

Status: Needs work » Needs review
znerol’s picture

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

davidgrayston’s picture

FileSize
13.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,951 pass(es). View

Thanks – patch updated with correction to @see docs

davidgrayston’s picture

FileSize
13.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,956 pass(es). View

That was slightly wrong - here's an updated patch

davidgrayston’s picture

FileSize
13.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,961 pass(es). View

Sorry, again that was wrong, here's an updated patch

alexpott’s picture

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

davidgrayston’s picture

FileSize
2.19 KB

Skipping out the mistakes, here's an interdiff between these patches:
2206571-30-phpunit-tests-config.patch
2206571-35-phpunit-tests-config.patch

znerol’s picture

Status: Needs review » Reviewed & tested by the community
+++ 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()

davidgrayston’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
13.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
2.62 KB

Yes, you are right, here's the corrected patch

alexpott’s picture

Status: Needs review » Fixed

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(

  • alexpott committed d229eed on 8.0.x
    Issue #2206571 by davidgrayston, znerol, amitgoyal: Add PHPUnit tests...

Status: Fixed » Needs work

The last submitted patch, 39: 2206571-39-phpunit-tests-config.patch, failed testing.

alexpott’s picture

Status: Needs work » Fixed

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

davidgrayston’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.