Bazaar

Merge lp:~vila/bzr/split-diff-tests into lp:bzr

  1. split-diff-tests
  2. Merge into bzr.dev
Proposed by Vincent Ladeuil
Status: Merged
Approved by: Vincent Ladeuil
Approved revision: no longer in the source branch.
Merged at revision: 6601
Proposed branch: lp:~vila/bzr/split-diff-tests
Merge into: lp:bzr
Diff against target: 136 lines (+43/-35)
2 files modified
bzrlib/diff.py (+13/-15)
bzrlib/tests/test_diff.py (+30/-20)
To merge this branch: bzr merge lp:~vila/bzr/split-diff-tests
Reviewer Review Type Date Requested Status
Richard Wilbur Approve
Review via email: mp+236818@code.launchpad.net

Commit message

Split some tests to be able to get finer grained failures

Description of the change

Some simple refactoring to get finer grained failures from tests.

I also noted that --forward-ed is not part of diff 3.3 and I'm wondering if
we really want to keep that option.

To post a comment you must log in.
Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

Thanks, Vincent, for the improved granularity and specificity of the tests. The use of scenarios is powerful!

Regarding '--forward-ed' not part of diff v3.3:
How does diff v3.3 treat a command line which includes the removed option '--forward-ed'? Does it quietly ignore it or abort with an error code?

In either case, since the user is explicitly specifying the option, it seems benign to continue to regard it as a valid format option--at least as long as we are still supporting diff v3.2.

The other question to consider is, "How long would it be useful to support the diff v3.2 behaviour?" Ubuntu 12.04 has diff v3.2 while the first Ubuntu Linux release which includes diff v3.3 is 13.10.

1. If diff v3.3 quietly ignores the removed option, then we might regard it as important to determine which version of GNU diff is installed in order to either warn the user that this option no longer specifies a format or substitute '-u'. Maybe both.
2. If diff v3.3 aborts with an error message, then this will likely be a self-correcting problem--the user who explicitly specified the option will learn it is no longer supported and cease specifying that option on next invocation.

Regardless of these considerations, I approve of this patch.

+1

review: Approve
Revision history for this message
Vincent Ladeuil (vila) wrote :

> Thanks, Vincent, for the improved granularity and specificity of the tests.
> The use of scenarios is powerful!
>
> Regarding '--forward-ed' not part of diff v3.3:
> How does diff v3.3 treat a command line which includes the removed option
> '--forward-ed'? Does it quietly ignore it or abort with an error code?

Good point ! The option appears to be still supported even if it's not mentioned in the man page nor the online help anymore.

>
> In either case, since the user is explicitly specifying the option, it seems
> benign to continue to regard it as a valid format option--at least as long as
> we are still supporting diff v3.2.

Agreed, I've removed the comment.

And for the future, I think you're right, once the option is not supported by diff anymore, our users will soon enough discover that the issue is with diff itself.

Revision history for this message
Vincent Ladeuil (vila) wrote :

sent to pqm by email

Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

On Mon, Oct 6, 2014 at 6:35 AM, Vincent Ladeuil <email address hidden> wrote:
>> Regarding '--forward-ed' not part of diff v3.3:
>> How does diff v3.3 treat a command line which includes the removed option
>> '--forward-ed'? Does it quietly ignore it or abort with an error code?
>
> Good point ! The option appears to be still supported even if it's not mentioned in the man page nor the online help anymore.

I posted a documentation regression bug (#18655) on the bug-diffutils
mailing list. The thread is in their archives[*]. They said they
dropped documentation from shorter manuals because they thought no one
in their right mind would use the option but that it is still
documented in the full manual.

Reference:
[*] http://lists.gnu.org/archive/html/bug-diffutils/2014-10/index.html

Revision history for this message
Vincent Ladeuil (vila) wrote :

Wow ! Thanks for tracking this !

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/diff.py'
2--- bzrlib/diff.py 2014-09-16 13:42:23 +0000
3+++ bzrlib/diff.py 2014-10-06 12:35:04 +0000
4@@ -286,21 +286,19 @@
5 finally:
6 oldtmpf.close() # and delete
7 newtmpf.close()
8- # Clean up. Warn in case the files couldn't be deleted
9- # (in case windows still holds the file open, but not
10- # if the files have already been deleted)
11- try:
12- os.remove(old_abspath)
13- except OSError, e:
14- if e.errno not in (errno.ENOENT,):
15- warning('Failed to delete temporary file: %s %s',
16- old_abspath, e)
17- try:
18- os.remove(new_abspath)
19- except OSError, e:
20- if e.errno not in (errno.ENOENT,):
21- warning('Failed to delete temporary file: %s %s',
22- new_abspath, e)
23+
24+ def cleanup(path):
25+ # Warn in case the file couldn't be deleted (in case windows still
26+ # holds the file open, but not if the files have already been
27+ # deleted)
28+ try:
29+ os.remove(path)
30+ except OSError, e:
31+ if e.errno not in (errno.ENOENT,):
32+ warning('Failed to delete temporary file: %s %s', path, e)
33+
34+ cleanup(old_abspath)
35+ cleanup(new_abspath)
36
37
38 def get_trees_and_branches_to_diff_locked(
39
40=== modified file 'bzrlib/tests/test_diff.py'
41--- bzrlib/tests/test_diff.py 2014-09-16 13:42:23 +0000
42+++ bzrlib/tests/test_diff.py 2014-10-06 12:35:04 +0000
43@@ -17,7 +17,6 @@
44 import os
45 from cStringIO import StringIO
46 import subprocess
47-import sys
48 import tempfile
49
50 from bzrlib import (
51@@ -32,12 +31,15 @@
52 tests,
53 transform,
54 )
55-from bzrlib.symbol_versioning import deprecated_in
56-from bzrlib.tests import features, EncodingAdapter
57-from bzrlib.tests.blackbox.test_diff import subst_dates
58 from bzrlib.tests import (
59 features,
60- )
61+ EncodingAdapter,
62+)
63+from bzrlib.tests.blackbox.test_diff import subst_dates
64+from bzrlib.tests.scenarios import load_tests_apply_scenarios
65+
66+
67+load_tests = load_tests_apply_scenarios
68
69
70 def udiff_lines(old, new, allow_binary=False):
71@@ -63,6 +65,29 @@
72 return lines
73
74
75+class TestDiffOptions(tests.TestCase):
76+
77+ def test_unified_added(self):
78+ """Check for default style '-u' only if no other style specified
79+ in 'diff-options'.
80+ """
81+ # Verify that style defaults to unified, id est '-u' appended
82+ # to option list, in the absence of an alternative style.
83+ self.assertEqual(['-a', '-u'], diff.default_style_unified(['-a']))
84+
85+
86+class TestDiffOptionsScenarios(tests.TestCase):
87+
88+ scenarios = [(s, dict(style=s)) for s in diff.style_option_list]
89+ style = None # Set by load_tests_apply_scenarios from scenarios
90+
91+ def test_unified_not_added(self):
92+ # Verify that for all valid style options, '-u' is not
93+ # appended to option list.
94+ ret_opts = diff.default_style_unified(diff_opts=["%s" % (self.style,)])
95+ self.assertEqual(["%s" % (self.style,)], ret_opts)
96+
97+
98 class TestDiff(tests.TestCase):
99
100 def test_add_nl(self):
101@@ -143,19 +168,6 @@
102 'old', ['boo\n'], 'new', ['goo\n'],
103 StringIO(), diff_opts=['-u'])
104
105- def test_default_style_unified(self):
106- """Check for default style '-u' only if no other style specified
107- in 'diff-options'.
108- """
109- # Verify that style defaults to unified, id est '-u' appended
110- # to option list, in the absence of an alternative style.
111- self.assertEqual(['-a', '-u'], diff.default_style_unified(["-a"]))
112- # Verify that for all valid style options, '-u' is not
113- # appended to option list.
114- for s in diff.style_option_list:
115- ret_opts = diff.default_style_unified(diff_opts=["%s" % (s,)])
116- self.assertEqual(["%s" % (s,)], ret_opts)
117-
118 def test_internal_diff_default(self):
119 # Default internal diff encoding is utf8
120 output = StringIO()
121@@ -1484,7 +1496,6 @@
122 def test_encodable_filename(self):
123 # Just checks file path for external diff tool.
124 # We cannot change CPython's internal encoding used by os.exec*.
125- import sys
126 diffobj = diff.DiffFromTool(['dummy', '@old_path', '@new_path'],
127 None, None, None)
128 for _, scenario in EncodingAdapter.encoding_scenarios:
129@@ -1502,7 +1513,6 @@
130 self.assert_(fullpath.startswith(diffobj._root + '/safe'))
131
132 def test_unencodable_filename(self):
133- import sys
134 diffobj = diff.DiffFromTool(['dummy', '@old_path', '@new_path'],
135 None, None, None)
136 for _, scenario in EncodingAdapter.encoding_scenarios: