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?)

CommentFileSizeAuthor
#6 interdiff.txt2.48 KBjhedstrom
#6 settings-phpunit-2035863-06.patch2.26 KBjhedstrom
#3 settings-phpunit-2035863-02.patch1.53 KBjhedstrom
#2 settings-phpunit-2035863-02.patch1.53 KBjhedstrom
#1 Settings-coverage-more.png276.63 KBYesCT
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT’s picture

YesCT CreditAttribution: YesCT commented
FileSize
Settings-coverage-more.png276.63 KB

coverage of Settings, example:
Settings-coverage-more.png

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?

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Status: Active » Needs review
FileSize
settings-phpunit-2035863-02.patch1.53 KB

Here's a first attempt at adding unit tests specific to this component.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
FileSize
settings-phpunit-2035863-02.patch1.53 KB

Here's a first attempt at adding unit tests specific to the settings component.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Nice!

+++ b/core/tests/Drupal/Tests/Component/Utility/SettingsTest.phpundefined
@@ -0,0 +1,53 @@
+    $this->assertEquals('3', $settings->get('three', '3'));
+    $this->assertNull($settings->get('four'));

Maybe a small assertion message would help.

+++ b/core/tests/Drupal/Tests/Component/Utility/SettingsTest.phpundefined
@@ -0,0 +1,53 @@
+    $singleton = $settings->getSingleton();
+    $this->assertEquals($singleton, $settings);

Aside from that you could also test getAll()

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Needs work
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Status: Needs work » Needs review
FileSize
settings-phpunit-2035863-06.patch2.26 KB
interdiff.txt2.48 KB

I added messages, and moved each method into a separate test. This has 100% coverage of Settings class.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Nice! Thank you very much.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom commented
Issue summary: View changes
Parent issue: » #1938068: Convert UnitTestBase to PHPUnit