( )⚙ D5514 test: change test's diff generation to use mdiff for nicer output

This is an archive of the discontinued Mercurial Phabricator instance.

test: change test's diff generation to use mdiff for nicer output
Needs RevisionPublic

Authored by sangeet259 on Jan 7 2019, 9:53 AM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Summary

The current diff being used by tests upon failing is not very good. Sometimes there is a lot of redundancy.

To see for yourself, follow these steps:

  1. Download this bundle : http://bit.ly/2DuJjsS
$ hg clone https://www.mercurial-scm.org/hg
$ cd hg/
$ hg unbundle af8efb83e1d6.hg
$ hg up a1e70c1dbec0^
$ g revert --all --rev a1e70c1dbec0
$ hg revert -i tests/test-bookmarks-pushpull.t # revert only the last three hunks (press 'n' for the first 8, and 'y' for the last three)
$ cd tests
$ hg diff --rev a1e70c1dbec0 # nice output from Mercurial
$ ./run-test.py test-bookmarks-pushpull.t # poor output from the test runner

If you couldn't follow those steps, you can simply check the diffs here:

--- /media/sangeet/Stuff/hgrepo/Octobus/MD4/tests/test-bookmarks-pushpull.t
+++ /media/sangeet/Stuff/hgrepo/Octobus/MD4/tests/test-bookmarks-pushpull.t.b2-binary.err
@@ -1086,8 +1086,7 @@
   pushing to $TESTTMP/issue4455-dest (glob)
   searching for changes
   no changes found
-  pushkey-abort: prepushkey hook exited with status 1
-  abort: exporting bookmark @ failed!
+  abort: prepushkey hook exited with status 1
   [255]

 #endif
@@ -1126,27 +1125,27 @@
   pushing to ssh://user@dummy/issue4455-dest
   searching for changes
   no changes found
+  remote: prepushkey hook exited with status 1
+  abort: push failed on remote
+  [255]
+
+#endif
+
+  $ hg -R ../issue4455-dest/ bookmarks
+  no bookmarks set
+
+Using http
+----------
+
+#if b2-pushkey
+  $ hg push -B @ http # bundle2+
+  pushing to http://localhost:$HGPORT/
+  searching for changes
+  no changes found
   remote: pushkey-abort: prepushkey hook exited with status 1
   abort: exporting bookmark @ failed!
   [255]

-#endif
-
-  $ hg -R ../issue4455-dest/ bookmarks
-  no bookmarks set
-
-Using http
-----------
-
-#if b2-pushkey
-  $ hg push -B @ http # bundle2+
-  pushing to http://localhost:$HGPORT/
-  searching for changes
-  no changes found
-  remote: pushkey-abort: prepushkey hook exited with status 1
-  abort: exporting bookmark @ failed!
-  [255]
-
   $ hg -R ../issue4455-dest/ bookmarks
   no bookmarks set

@@ -1166,8 +1165,8 @@
   pushing to ssh://user@dummy/issue4455-dest
   searching for changes
   no changes found
-  remote: pushkey-abort: prepushkey hook exited with status 1
-  abort: exporting bookmark @ failed!
+  remote: prepushkey hook exited with status 1
+  abort: push failed on remote
   [255]

 #endif

ERROR: test-bookmarks-pushpull.t (case b2-binary) output changed
!.
Failed test-bookmarks-pushpull.t (case b2-binary): output changed
# Ran 2 tests, 0 skipped, 1 failed.
python hash seed: 133193196

diff -r a1e70c1dbec0 tests/test-bookmarks-pushpull.t
--- a/tests/test-bookmarks-pushpull.t	Thu Nov 01 22:20:00 2018 +0530
+++ b/tests/test-bookmarks-pushpull.t	Tue Oct 17 12:38:13 2017 +0200
@@ -1086,8 +1086,7 @@
   pushing to $TESTTMP/issue4455-dest (glob)
   searching for changes
   no changes found
-  pushkey-abort: prepushkey hook exited with status 1
-  abort: exporting bookmark @ failed!
+  abort: prepushkey hook exited with status 1
   [255]

 #endif
@@ -1126,8 +1125,8 @@
   pushing to ssh://user@dummy/issue4455-dest
   searching for changes
   no changes found
-  remote: pushkey-abort: prepushkey hook exited with status 1
-  abort: exporting bookmark @ failed!
+  remote: prepushkey hook exited with status 1
+  abort: push failed on remote
   [255]

 #endif
@@ -1166,8 +1165,8 @@
   pushing to ssh://user@dummy/issue4455-dest
   searching for changes
   no changes found
-  remote: pushkey-abort: prepushkey hook exited with status 1
-  abort: exporting bookmark @ failed!
+  remote: prepushkey hook exited with status 1
+  abort: push failed on remote
   [255]

 #endif

Tests uses _unified_diff based on difflib.py . The output isn't great!

Mercurial's diff uses mdiff which gives better diffs.
This patch uses mdiff's unidiff function to generate the diff for the failed tests.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

sangeet259 created this revision.Jan 7 2019, 9:53 AM
sangeet259 updated this revision to Diff 13051.Jan 7 2019, 9:59 AM

Note: To see this in action, one needs to add the changes from this mdiff.py to the mdiff.py of their installed mercurial.
This is because unless you do that, you cannot import mdiff.new_patch as that function won't be there in your installed version, and hence it will keep falling back to the unified_diff.

Else, can it be implemented in any other way.

durin42 requested changes to this revision.Jan 8 2019, 11:47 AM
durin42 added a subscriber: durin42.
durin42 added inline comments.
tests/run-tests.py
71

This will fail if Mercurial isn't already globally installed on the machine, as would be the case in at least some packaging circumstances when building from a source tarball.

Probably need a try/except, and _maybe_ a sanity check that mdiff's API matches what we expect?

This revision now requires changes to proceed.Jan 8 2019, 11:47 AM
pulkit added a subscriber: pulkit.Jan 8 2019, 12:27 PM

If you couldn't follow those steps, you can simply check the diffs here:
test's diff: https://pastebin.com/Z4LRg4vx
diff used by hg diff: https://pastebin.com/qanQEsiA

Can you make the diffs a part of commit message since pastebin's pastes can be removed in future?

@durin42 So the try/except will fall back to unified diff?
Is there a way we can enforce this on system's that don't have mercurial installed globally and not have to fall back on the earlier practice.

Also, I didn't get your comment on checking the API of mdiff :/

@durin42 So the try/except will fall back to unified diff?

Correct.

Is there a way we can enforce this on system's that don't have mercurial installed globally and not have to fall back on the earlier practice.

I think we don't have any choice: if hg isn't installed, we have to fall back to the old codepath, since we won't know if mdiff is present.

Also, I didn't get your comment on checking the API of mdiff :/

Well, what if we iterate on the API of mdiff.new_diff? then again, if the point of it is to have the same API as difflib.unified_diff, probably don't need to worry (and so we only have to fall back to difflib if mdiff isn't available or is too old).

sangeet259 edited the summary of this revision. (Show Details)Jan 10 2019, 3:55 AM
sangeet259 updated this revision to Diff 13123.
durin42 added inline comments.Jan 10 2019, 10:17 AM
mercurial/mdiff.py
554

At least add a docstring taht explains this is designed to be a similar API to difflib.unified_diff?

tests/run-tests.py
50

unused?

54

unused?

80

Nit: except (ImportError, AttributeError): instead of just catching Exception.

sangeet259 updated this revision to Diff 13998.Feb 9 2019, 11:48 PM
sangeet259 marked 3 inline comments as done.Feb 9 2019, 11:51 PM
sangeet259 marked an inline comment as done.
This comment was removed by sangeet259.

Hey @durin42 , take a look at the changes when you get time.

I've done some cleanups (dropped an unused import, copyedited the commit message a bit) and should push this soon. Thanks!

Even with my in-flight fixes, I can't get this to use the non-unified_diff codepath. Can you test again?

https://bitbucket.org/snippets/durin42/nejj94 has my in-flight fixes, but you may find it challenging to apply (the test output in the commit message confuses the patch parser). Hopefully that helps...

tests/run-tests.py
80

In the future please *run tests* before requesting another round of review: this is a trivial syntax error and proves you didn't try running tests.

Apologies, I didn't run the tests after the latest addition. 😔

durin42 requested changes to this revision.Feb 20 2019, 3:21 PM

(moving this off the dashboard until it gets updated)

This revision now requires changes to proceed.Feb 20 2019, 3:21 PM