Problem/Motivation

Yet another notch in the belt of #2052109: [meta] Expand phpunit tests for \Drupal\Component\Plugin classes.

This issue covers Drupal\Component\Plugin\Context\Context, which has zero unit test coverage.

In the process of writing this test, I found a bug: #2378297: Inconsistent use of ContextDefinitionInterface in Drupal\Component\Plugin\Context\Context::getConstraints() This means that getConstraints() is not covered by this test, and should be covered by the bug fix in that other issue.

Proposed resolution

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Unfrozen changes Unfrozen because it only changes automated tests.
CommentFileSizeAuthor
#9 interdiff_1.txt472 bytesmile23
#9 2378311_9.patch3.13 KBmile23
#3 interdiff.txt2.1 KBmile23
#3 2378311_3.patch3.82 KBmile23
#1 2378311_1.patch3.18 KBmile23

Comments

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Active » Needs review
StatusFileSize
new 2378311_1.patch3.18 KB

This patch covers Context::getContextValue(), which is the most complex method in this class (CRAP score: 12).

As noted in the issue summary, this patch avoids getConstraints(), which is buggy.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/tests/Drupal/Tests/Component/Plugin/Context/ContextTest.php
@@ -0,0 +1,90 @@
+    if ($context_value) {
...
+    else {

Can't we split this up into two test methods and use two times the same provider?

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
StatusFileSize
new 2378311_3.patch3.82 KB
new interdiff.txt2.1 KB

The logic follows the logic of getContextValue().

I thought about refactoring it the way you mention, but it doesn't make much sense, since there are only three data sets from the data provider.

Nevertheless: Judge for yourself. :-)

./vendor/bin/phpunit --filter=ContextTest
PHPUnit 4.1.4 by Sebastian Bergmann.

Configuration read from /Users/paulmitchum/projects/drupal8/core/phpunit.xml.dist

.SSS..

Time: 4.77 seconds, Memory: 98.50Mb

OK, but incomplete, skipped, or risky tests!
Tests: 6, Assertions: 8, Skipped: 3.

I prefer the patch in #1.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Issue summary: View changes

Anyone? :-)

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Issue summary: View changes
duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented

6/6 tests PASS
29% coverage but most of the uncovered code is made of getters and setters.
Don't we want the ::getConstraints() and ::validate() methods tested ?

Usual comments:

  1. +++ b/core/tests/Drupal/Tests/Component/Plugin/Context/ContextTest.php
@@ -0,0 +1,112 @@
+use Drupal\Component\Plugin\Context\Context;

    Unused ;) 

  2. +++ b/core/tests/Drupal/Tests/Component/Plugin/Context/ContextTest.php
@@ -0,0 +1,112 @@
+    if ($context_value) {
...
+    if (!$context_value) {

    I don't like that kind of condition but that's just my point of view...

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

See the summary about getConstraints(). It's buggy, so I didn't test it :-) #2378297: Inconsistent use of ContextDefinitionInterface in Drupal\Component\Plugin\Context\Context::getConstraints()

validate() depends on a static factory. It should really allow you to inject a validator, but it doesn't.

Point 2: That's what I mean about the patch in #1 being better.

daffie’s picture

daffie commented

@Mile23: If you think that the validate() should use injection instead of a static factory, why don't you create an issue for that.

I prefer the patch from comment #1. I have added to #2378297: Inconsistent use of ContextDefinitionInterface in Drupal\Component\Plugin\Context\Context::getConstraints() that there is the need for a PHPUnit test for the ::getConstraints() method.

Fix the first comment from DuaelFr: remove the unused: use Drupal\Component\Plugin\Context\Context;.

If there is an issue created for the ::validate() method and in that issue an mention for creating a PHPUnit test for the ::validate() method, then I shall give this issue an RTBC.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
StatusFileSize
new 2378311_9.patch3.13 KB
new interdiff_1.txt472 bytes

Here's the patch from comment #1, minus the use statement. :-)

Issue about validate() to come.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs review » Needs work
mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review

Hello, testbot?

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Related issues: +#2382805: Modify Drupal\Component\Plugin\Context\ContextInterface to allow for unit testing.

Filed #2382805: Modify Drupal\Component\Plugin\Context\ContextInterface to allow for unit testing..

One or the other of these two issues will need a re-roll when the other drops.

daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

In this patch is the file ContextText class added with a PHPUnit test. The only function that is been tested is getContextValue(). There are also a few getters and setters functions that are not tested. The two remaining functions are getConstraints() and validate(). For the first function is #2378297: Inconsistent use of ContextDefinitionInterface in Drupal\Component\Plugin\Context\Context::getConstraints() created and for the second function #2382805: Modify Drupal\Component\Plugin\Context\ContextInterface to allow for unit testing.. The patch lloks good to me and I give it a RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 2378311_9.patch, failed testing.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

Patch applies and everything passes locally so I assume it's the testbot.

Mile23 queued 9: 2378311_9.patch for re-testing.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Needs work » Needs review
daffie’s picture

daffie commented
Status: Needs review » Reviewed & tested by the community

Before the retest failed it was RTBC. Nothing has changed and the testbot is happy again. So back to RTBC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Not really the most qualified to give these sign-off, but they've been sitting here long enough for someone to raise objections, and moar test coverage is never a bad thing.

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 559ce19 on 
    Issue #2378311 by Mile23, daffie: Expand unit testing for Drupal\...

Status: Fixed » Closed (fixed)

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