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

Comments

yesct’s picture

yesct commented
StatusFileSize
new 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
commented
Status: Active » Needs review
StatusFileSize
new 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
commented
StatusFileSize
new 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
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
commented
Status: Needs review » Needs work
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
commented
Status: Needs work » Needs review
StatusFileSize
new settings-phpunit-2035863-06.patch2.26 KB
new 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
commented
Status: Needs review » Reviewed & tested by the community

Nice! Thank you very much.

catch’s picture

catch
he/him
Primary language English
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
commented
Issue summary: View changes
Parent issue: » #1938068: Convert UnitTestBase to PHPUnit