testtools

Merge lp:~lifeless/testtools/0.9.1 into lp:~testtools-dev/testtools/0.9

  1. 0.9.1
  2. Merge into 0.9
Proposed by Robert Collins
Status: Merged
Approved by: Jonathan Lange
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~lifeless/testtools/0.9.1
Merge into: lp:~testtools-dev/testtools/0.9
Diff against target: 193 lines (+65/-30)
5 files modified
NEWS (+10/-0)
testtools/matchers.py (+33/-15)
testtools/testcase.py (+3/-2)
testtools/tests/test_matchers.py (+9/-6)
testtools/tests/test_testtools.py (+10/-7)
To merge this branch: bzr merge lp:~lifeless/testtools/0.9.1
Reviewer Review Type Date Requested Status
Jonathan Lange Approve
Martin Pool (community) Needs Fixing
Review via email: mp+15256@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Robert Collins (lifeless) wrote :

Better API for Matcher.

Revision history for this message
Martin Pool (mbp) wrote :

You're inconsistent about MisMatch vs Mismatch. Since it's a single English word I would prefer the latter.

107 - if matcher.matches(matchee):
108 + match = matcher.match(matchee)
109 + if not match:
110 return
111 self.fail('Match failed. Matchee: "%s"\nMatcher: %s\nDifference: %s\n'
112 - % (matchee, matcher, matcher.describe_difference(matchee)))
113 + % (matchee, matcher, match.describe()))
114

I would call the variable 'mismatch' otherwise the sense looks inverted.

review: Needs Fixing
Revision history for this message
Robert Collins (lifeless) wrote :

On Thu, 2009-11-26 at 05:29 +0000, Martin Pool wrote:
> Review: Needs Fixing
> You're inconsistent about MisMatch vs Mismatch. Since it's a single English word I would prefer the latter.

I am?.... oh, DocTestMisMatch - sure, fixed.

> 107 - if matcher.matches(matchee):
> 108 + match = matcher.match(matchee)
> 109 + if not match:
> 110 return
> 111 self.fail('Match failed. Matchee: "%s"\nMatcher: %s\nDifference: %s\n'
> 112 - % (matchee, matcher, matcher.describe_difference(matchee)))
> 113 + % (matchee, matcher, match.describe()))
> 114
>
> I would call the variable 'mismatch' otherwise the sense looks inverted.

Done - thanks for the review.

-Rob

lp:~lifeless/testtools/0.9.1 updated
34. By Robert Collins

Clarity tweaks from Martin.

Revision history for this message
Jonathan Lange (jml) wrote :

Ahh, I think this API is much more sensible, and wish I'd thought of this earlier.

 * In the NEWS file you say "sideffects" to mean "side effects"
 * In Mismatch.describe's docstring, "human readable" should say "human-readable"
 * I think saying "assertIsNot" is nicer than "assertNotEqual" when contrasting with None, but whatever.

Please feel free to land the patch after fixing the first two points.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-11-21 19:24:13 +0000
3+++ NEWS 2009-11-26 05:44:09 +0000
4@@ -1,6 +1,16 @@
5 testtools NEWS
6 ==============
7
8+0.9.1
9+~~~~~
10+
11+The new matcher API introduced in 0.9.0 had a small flaw where the matchee
12+would be evaluated twice to get a description of the mismatch. This could
13+lead to bugs if the act of matching caused side effects to occur in the
14+matchee. While having such sideffects isn't desirable, as we could change the
15+API to make it robust even in the presence of such side effects, we have done
16+so now before the API has become widespread.
17+
18 0.9.0
19 ~~~~~
20
21
22=== modified file 'testtools/matchers.py'
23--- testtools/matchers.py 2009-11-20 00:46:00 +0000
24+++ testtools/matchers.py 2009-11-26 05:44:09 +0000
25@@ -21,8 +21,9 @@
26 class Matcher:
27 """A pattern matcher.
28
29- A Matcher must implement matches, __str__ and describe_difference to be
30- used by testtools.TestCase.assertThat.
31+ A Matcher must implement match and __str__ to be used by
32+ testtools.TestCase.assertThat. Matcher.match(thing) returns None when
33+ thing is completely matched, and a Mismatch object otherwise.
34
35 Matchers can be useful outside of test cases, as they are simply a
36 pattern matching language expressed as objects.
37@@ -31,9 +32,10 @@
38 a Java transcription.
39 """
40
41- def matches(self, something):
42- """Returns True if this matcher matches something, False otherwise."""
43- raise NotImplementedError(self.matches)
44+ def match(self, something):
45+ """Return None if this matcher matches something, a Mismatch otherwise.
46+ """
47+ raise NotImplementedError(self.match)
48
49 def __str__(self):
50 """Get a sensible human representation of the matcher.
51@@ -43,10 +45,14 @@
52 """
53 raise NotImplementedError(self.__str__)
54
55- def describe_difference(self, something):
56- """Describe why something did not match.
57+
58+class Mismatch:
59+ """An object describing a mismatch detected by a Matcher."""
60+
61+ def describe(self):
62+ """Describe the mismatch.
63
64- This should be either human readable or castable to a string.
65+ This should be either a human readable string or castable to a string.
66 """
67 raise NotImplementedError(self.describe_difference)
68
69@@ -80,10 +86,22 @@
70 result += '\n'
71 return result
72
73- def matches(self, actual):
74- return self._checker.check_output(self.want, self._with_nl(actual),
75- self.flags)
76-
77- def describe_difference(self, actual):
78- return self._checker.output_difference(self, self._with_nl(actual),
79- self.flags)
80+ def match(self, actual):
81+ with_nl = self._with_nl(actual)
82+ if self._checker.check_output(self.want, with_nl, self.flags):
83+ return None
84+ return DocTestMismatch(self, with_nl)
85+
86+ def _describe_difference(self, with_nl):
87+ return self._checker.output_difference(self, with_nl, self.flags)
88+
89+
90+class DocTestMismatch:
91+ """Mismatch object for DocTestMatches."""
92+
93+ def __init__(self, matcher, with_nl):
94+ self.matcher = matcher
95+ self.with_nl = with_nl
96+
97+ def describe(self):
98+ return self.matcher._describe_difference(self.with_nl)
99
100=== modified file 'testtools/testcase.py'
101--- testtools/testcase.py 2009-11-20 00:46:00 +0000
102+++ testtools/testcase.py 2009-11-26 05:44:09 +0000
103@@ -191,10 +191,11 @@
104 :param matcher: An object meeting the testtools.Matcher protocol.
105 :raises self.failureException: When matcher does not match thing.
106 """
107- if matcher.matches(matchee):
108+ mismatch = matcher.match(matchee)
109+ if not mismatch:
110 return
111 self.fail('Match failed. Matchee: "%s"\nMatcher: %s\nDifference: %s\n'
112- % (matchee, matcher, matcher.describe_difference(matchee)))
113+ % (matchee, matcher, mismatch.describe()))
114
115 def expectFailure(self, reason, predicate, *args, **kwargs):
116 """Check that a test fails in a particular way.
117
118=== modified file 'testtools/tests/test_matchers.py'
119--- testtools/tests/test_matchers.py 2009-11-20 00:46:00 +0000
120+++ testtools/tests/test_matchers.py 2009-11-26 05:44:09 +0000
121@@ -15,14 +15,16 @@
122
123 class TestDocTestMatchesInterface(TestCase):
124
125- def test_matches_matches(self):
126+ def test_matches_match(self):
127 matcher = DocTestMatches("Ran 1 test in ...s", doctest.ELLIPSIS)
128 matches = ["Ran 1 test in 0.000s", "Ran 1 test in 1.234s"]
129 mismatches = ["Ran 1 tests in 0.000s", "Ran 2 test in 0.000s"]
130 for candidate in matches:
131- self.assertTrue(matcher.matches(candidate))
132+ self.assertEqual(None, matcher.match(candidate))
133 for candidate in mismatches:
134- self.assertFalse(matcher.matches(candidate))
135+ mismatch = matcher.match(candidate)
136+ self.assertNotEqual(None, mismatch)
137+ self.assertNotEqual(None, getattr(mismatch, 'describe', None))
138
139 def test__str__(self):
140 # [(expected, object to __str__)].
141@@ -35,11 +37,12 @@
142
143 def test_describe_difference(self):
144 # [(expected, matchee, matcher), ...]
145- examples = [('Expected:\n Ran 1 test in ...s\nGot:\n'
146+ examples = [('Expected:\n Ran 1 tests in ...s\nGot:\n'
147 ' Ran 1 test in 0.123s\n', "Ran 1 test in 0.123s",
148- DocTestMatches("Ran 1 test in ...s", doctest.ELLIPSIS))]
149+ DocTestMatches("Ran 1 tests in ...s", doctest.ELLIPSIS))]
150 for difference, matchee, matcher in examples:
151- self.assertEqual(difference, matcher.describe_difference(matchee))
152+ mismatch = matcher.match(matchee)
153+ self.assertEqual(difference, mismatch.describe())
154
155
156 class TestDocTestMatchesSpecific(TestCase):
157
158=== modified file 'testtools/tests/test_testtools.py'
159--- testtools/tests/test_testtools.py 2009-11-19 10:56:58 +0000
160+++ testtools/tests/test_testtools.py 2009-11-26 05:44:09 +0000
161@@ -235,22 +235,25 @@
162
163 def test_assertThat_matches_clean(self):
164 class Matcher:
165- def matches(self, foo):
166- return True
167+ def match(self, foo):
168+ return None
169 self.assertThat("foo", Matcher())
170
171 def test_assertThat_mismatch_raises_description(self):
172 calls = []
173+ class Mismatch:
174+ def __init__(self, thing):
175+ self.thing = thing
176+ def describe(self):
177+ calls.append(('describe_diff', self.thing))
178+ return "object is not a thing"
179 class Matcher:
180- def matches(self, thing):
181+ def match(self, thing):
182 calls.append(('match', thing))
183- return False
184+ return Mismatch(thing)
185 def __str__(self):
186 calls.append(('__str__',))
187 return "a description"
188- def describe_difference(self, thing):
189- calls.append(('describe_diff', thing))
190- return "object is not a thing"
191 class Test(TestCase):
192 def test(self):
193 self.assertThat("foo", Matcher())

Subscribers

People subscribed via source and target branches

to all changes: