Problem/Motivation

Some work has been done on the DiffEngine file. It contains a lot of algorithms and is therefore well suited to being unit tested.

I created the test file while working on issue #2079863: Remove Unused local variable from /core/lib/Drupal/Component Utility/Crypt.php Transliteration/PHPTransliteration.php Utility/UserAgent.php Gettext/PoHeader.php removing unused variables

Proposed resolution

- create a Tests/Component/Diff folder
- add a DiffEngineTest file
- test the edits attribute after creating a Diff on two simple line arrays

Follow-up from #2079863: Remove Unused local variable from /core/lib/Drupal/Component Utility/Crypt.php Transliteration/PHPTransliteration.php Utility/UserAgent.php Gettext/PoHeader.php.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fabricebernhard’s picture

Status: Active » Needs review
FileSize
2.1 KB

This is the basic test I added:

        $diff = new \Diff($this->lines1, $this->lines2);

        $expected = array(
            new \_DiffOp_Copy(array('Hello World')),
            new \_DiffOp_Add(array('I am a new line')),
            new \_DiffOp_Copy(array('I am an original line'))
        );

        $this->assertCount(count($expected), $diff->edits);

        foreach($diff->edits as $key => $diffObject) {
            $this->assertInstanceOf(get_class($expected[$key]), $diffObject);
            $this->assertSame($expected[$key]->orig, $diffObject->orig);
            $this->assertSame($expected[$key]->closing, $diffObject->closing);
        }

I would welcome comments on the require_once part:

        define('DRUPAL_ROOT', __DIR__ .'/../../../../../../');
        require_once DRUPAL_ROOT . '/core/lib/Drupal/Component/Diff/DiffEngine.php';

The Diff class is not autoloaded. I could add an issue to fix this later, but in the meantime is that ok?

Status: Needs review » Needs work

The last submitted patch, 1: drupal-core-add-diffengine-test-2156677-1.patch, failed testing.

The last submitted patch, 1: drupal-core-add-diffengine-test-2156677-1.patch, failed testing.

fabricebernhard’s picture

Title: Add test coverage for /core/lib/Drupal/Component/Diff/DiffEngine.php » Add PHPUnit test coverage for /core/lib/Drupal/Component/Diff/DiffEngine.php
FileSize
2.1 KB

Patch in wrong direction... Corrected.

fabricebernhard’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 4: drupal-core-add-diffengine-test-2156677-2.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review
FileSize
3.3 KB
4.94 KB

Fixed some PHP 4-isms in DiffEngine.php, and wrangled the namespace problems. Moved data to a data provider.

Mile23’s picture

Hmm. No testbot for me.

The last submitted patch, 7: 2156677_2.patch, failed testing.

Mile23’s picture

FileSize
3.01 KB
1.25 KB

Got rid of namespace changes.

The last submitted patch, 10: 2156677_10.patch, failed testing.

Mile23’s picture

Hmm. Yet another case of 'it passed locally.'

bryanbraun’s picture

I ran the tests locally against a fresh D8, and it looked like some Simpletest tests (specifically, for Import UI & Comment Entity Cache Tags) stopped failing once I removed the changes to DiffEngine.php. The PHPUnit tests are working for me. I've resubmitted the patch for testing.

lhangea’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed and tested.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

That require_once line had me hiding under the covers :D so I investigated this a little. The DiffEngine.php file says it came from phpwiki. I have a number of questions:

a) Why is this in core/lib/Drupal/Component, rather than core/vendor?
b) Why are we unit testing this, versus phpwiki doing it? Should this be an upstream patch instead?

Mile23’s picture

Annotation tells us that the Drupal version of DiffEngine comes from phpwiki 1.3.3. phpwiki is hosted on sourceforge, and uses SVN. Horror.

Note that our version has a DrupalDiffFormatter class in the file.

This code appears to only be used by the Config component, subclassed as DrupalDiffFormatter. There might be contrib that needs it, or maybe IMP.

There's no packagist record for it, in any useful form.

I think its ours now...

And if that's the case, it needs some coding standards love.

Or, really, actually, what it needs is someone to make it into a maintained library that can be installed through composer, because other projects have forked it, too: https://github.com/wikimedia/mediawiki-core/blob/master/includes/diff/Da...

Or..... Who knows? We could refactor to use a library that's already present, such as this one: https://github.com/sebastianbergmann/diff (It's a dependency of PHPUnit).

How to proceed?

lhangea’s picture

Actually a contrib module that needs this library is D8 version of Diff module.

lhangea’s picture

Hi,
I've been selected in Google Summer of Code 2014 with the project Port Diff module to Drupal 8. As I said above Diff module needs this DiffEngine (at least D7 version of Diff uses it). What do you guys think we should do with this ? I need to know this relatively soon. If we decide to keep it as it is I can do some work on coding standards if needed.

filijonka’s picture

Status: Needs review » Closed (won't fix)
YesCT’s picture

Issue summary: View changes

using [#NNNN] in issue summary