Problem/Motivation

The \Drupal\Component\Diff code has no PHPUnit test coverage (and only secondary tests from the config module).

Proposed resolution

Remaining tasks

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom’s picture

Issue summary: View changes
jhedstrom’s picture

jhedstrom’s picture

Issue summary: View changes
jhedstrom’s picture

Assigned: jhedstrom » Unassigned
Mile23’s picture

Status: Active » Needs review
FileSize
5.52 KB

Try this on.

CRAP scores:

DiffOp 132 -> 26

HWLDFWordAccumulator also 132 -> 26

DiffEngine 12,210 -> 4,318

I only tested public methods using no mocking. Some of the higher complexity public methods look like they might not be used by core. Diff::check(), in fact, says it's only for debugging purposes.

Also verified that patch #76 in #2337283: Add a composer.json file to every component requires drupal/utility for this component.

Some of the higher CRAP score methods really need some refactoring. That's only possible if we test them. ::gulp::

Status: Needs review » Needs work

The last submitted patch, 5: 2472633_5.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review
FileSize
5.54 KB
615 bytes

Typo. Amazing how PHPUnit can find it but run-tests.sh can't. :-)

dawehner’s picture

  1. +++ b/core/tests/Drupal/Tests/Component/Diff/Engine/DiffEngineTest.php
    @@ -0,0 +1,89 @@
    +      [[], [], []],
    ...
    +      [
    ...
    +      [
    ...
    +      [
    

    Let's use string keys to explain the different testcases better

  2. +++ b/core/tests/Drupal/Tests/Component/Diff/Engine/DiffOpTest.php
    @@ -0,0 +1,36 @@
    +   * @covers ::reverse
    +   * @expectedException \PHPUnit_Framework_Error
    +   */
    

    Note: The recommended way is now to not use the annotation but rather the setExpectedException method, sorry

Mile23’s picture

#8.1: Not sure what you mean. Read the test to see what it does.

#8.2: Doesn't make any sense, but OK.

dawehner’s picture

#8.1: Not sure what you mean. Read the test to see what it does.

This is entirely not the point. The point is having a MUCh easier time to quickly scan it, for example when you have a test failure.
PhpUnit is showing those keys additional.

Mile23’s picture

+++ b/core/tests/Drupal/Tests/Component/Diff/Engine/DiffEngineTest.php
@@ -31,11 +31,11 @@ class DiffEngineTest extends \PHPUnit_Framework_TestCase {
+      'empty' => [[], [], []],

OK, gotcha. +1

Now who will RTBC since you patched it? :-)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Come on, I just changed array keys :)

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

  • catch committed 4903975 on 8.1.x
    Issue #2472633 by Mile23, dawehner: Expand PHPUnit test coverage of the...

  • catch committed 0240b6d on 8.0.x
    Issue #2472633 by Mile23, dawehner: Expand PHPUnit test coverage of the...

Status: Fixed » Closed (fixed)

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