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
Comment | File | Size | Author |
---|---|---|---|
#13 | interdiff-2156677-10-13.txt | 607 bytes | bryanbraun |
#13 | diffengine-unit-tests-2156677-13.patch | 2.22 KB | bryanbraun |
#10 | interdiff.txt | 1.25 KB | Mile23 |
#10 | 2156677_10.patch | 3.01 KB | Mile23 |
#7 | interdiff.txt | 4.94 KB | Mile23 |
Comments
Comment #1
fabricebernhard CreditAttribution: fabricebernhard commentedThis is the basic test I added:
I would welcome comments on the require_once part:
The Diff class is not autoloaded. I could add an issue to fix this later, but in the meantime is that ok?
Comment #4
fabricebernhard CreditAttribution: fabricebernhard commentedPatch in wrong direction... Corrected.
Comment #5
fabricebernhard CreditAttribution: fabricebernhard commentedComment #7
Mile23Fixed some PHP 4-isms in DiffEngine.php, and wrangled the namespace problems. Moved data to a data provider.
Comment #8
Mile23Hmm. No testbot for me.
Comment #10
Mile23Got rid of namespace changes.
Comment #12
Mile23Hmm. Yet another case of 'it passed locally.'
Comment #13
bryanbraun CreditAttribution: bryanbraun commentedI 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.
Comment #14
lhangea CreditAttribution: lhangea commentedReviewed and tested.
Comment #15
webchickThat 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?
Comment #16
Mile23Annotation 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?
Comment #17
lhangea CreditAttribution: lhangea commentedActually a contrib module that needs this library is D8 version of Diff module.
Comment #18
lhangea CreditAttribution: lhangea commentedHi,
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.
Comment #19
filijonka CreditAttribution: filijonka commentedclosing this issue due to #1848266: Convert Diff into a proper, PSR-0-compatible PHP component.
Comment #20
YesCT CreditAttribution: YesCT commentedusing [#NNNN] in issue summary