Merge lp:~mbp/subunit/408821-filter-re into lp:~subunit/subunit/trunk
- 408821-filter-re
- Merge into trunk
Status: | Superseded | ||||
---|---|---|---|---|---|
Proposed branch: | lp:~mbp/subunit/408821-filter-re | ||||
Merge into: | lp:~subunit/subunit/trunk | ||||
Diff against target: | None lines | ||||
To merge this branch: | bzr merge lp:~mbp/subunit/408821-filter-re | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Subunit Developers | Pending | ||
Review via email: mp+9680@code.launchpad.net |
This proposal has been superseded by a proposal from 2009-08-06.
Commit message
Description of the change
Martin Pool (mbp) wrote : | # |
Martin Pool (mbp) wrote : | # |
> I anticipate it being very
> useful.
yep, it is :)
Robert Collins (lifeless) wrote : | # |
A few thoughts. Broadly this is fine and I like the feature.
Rather than document in README, perhaps extending the help (module
docstring) of subunit-filter would be more accessible and provide a home
for other such one-command things. OTOH we probably need a manual at
some point anyway, so up to you.
Rather than --[anti]-match, I'd rather something like:
--with RE
--without RE
Related to that, I wonder if without is really needed - the (?!...) can
embed negative patterns. I'm fine with it, just speculating.
In the internals though, I think it would make sense to have a single
re:
patterns = []
for pattern in options.
patterns.
for pattern in options.
patterns.
rule = "|".join(patterns)
The interface is a little awkward - you use a functional style, but it
doesn't fit all that well. For instance - "str(None) -> 'None'". So,
your code will incorrectly match on a pattern like 'one'.than
I think I'd prefer it if, in the Filter result you did:
should_filter = self._check_
and that function (not a lambda) does
if not callable(
return False
else:
if info is None:
info = ''
else:
info = str(info)
return self._filter_
And that way folk writing predicates can be sure they are dealing with
strings - we centralise that knowledge.
Lastly, and this can come later if you like, I'd really like the ability
to specify multiple patterns, which optparse can do quite easily with
store="append".
-Rob
Martin Pool (mbp) wrote : | # |
> A few thoughts. Broadly this is fine and I like the feature.
>
> Rather than document in README, perhaps extending the help (module
> docstring) of subunit-filter would be more accessible and provide a home
> for other such one-command things. OTOH we probably need a manual at
> some point anyway, so up to you.
I think you need some overall "here's what you can do with it" documentation, so I put it there.
>
> Rather than --[anti]-match, I'd rather something like:
> --with RE
> --without RE
ok
>
> Related to that, I wonder if without is really needed - the (?!...) can
> embed negative patterns. I'm fine with it, just speculating.
jeff's mum will never guess that.
> In the internals though, I think it would make sense to have a single
> re:
> patterns = []
> for pattern in options.
> patterns.
> for pattern in options.
> patterns.
> rule = "|".join(patterns)
I think it's much more useful to have (A AND NOT B) than (A OR NOT B) as far as cutting down what's present in the output.
> The interface is a little awkward - you use a functional style, but it
> doesn't fit all that well. For instance - "str(None) -> 'None'".
That seems like a non sequiter.
And I guess I see subunit as generally wanting to be functional.
> So,
> your code will incorrectly match on a pattern like 'one'.than
I don't think so, unless the test can be None.
>
> I think I'd prefer it if, in the Filter result you did:
> should_filter = self._check_
>
> and that function (not a lambda) does
> if not callable(
> return False
> else:
> if info is None:
> info = ''
> else:
> info = str(info)
> return self._filter_
1- Python sucks :-/, if I wanted 'if (a!=NULL)' everywhere I'd use C :-}
2- I think it's useful to allow future code to provide filters that look at objects not strings, seeing as the rest of it deals with objects. I realize they will mostly be lame objects that just wrap strings, so perhaps it's not worth it. If I was looking at bzr errors in-process I might want to poke through their attributes. For instance you might want to pass isinstance(err, KnownFailure).
3- Doing nothing if an object is not the type you expect is pretty error prone.
4- Looking at it now I'd like to fold *all* the filtering into evaluation of a predicate, so that this code was just changing one method not ~five. But that's a larger change and ties to your thing of how the status should be handled.
Sorry if this sounds grumpy. Thanks for the fast review.
> And that way folk writing predicates can be sure they are dealing with
> strings - we centralise that knowledge.
>
> Lastly, and this can come later if you like, I'd really like the ability
> to specify multiple patterns, which optparse can do quite easily with
> store="append".
Agree, but later.
- 79. By Martin Pool
-
Clean up and rename subunit-filter --with
- 80. By Martin Pool
-
Help message for --with/without
- 81. By Martin Pool
-
Build regexp filter as a closure not using globals
Unmerged revisions
Preview Diff
1 | === modified file 'NEWS' | |||
2 | --- NEWS 2009-08-02 22:55:36 +0000 | |||
3 | +++ NEWS 2009-08-05 06:57:48 +0000 | |||
4 | @@ -30,6 +30,9 @@ | |||
5 | 30 | stream to a single JUnit style XML stream using the pyjunitxml | 30 | stream to a single JUnit style XML stream using the pyjunitxml |
6 | 31 | python library. | 31 | python library. |
7 | 32 | 32 | ||
8 | 33 | * ``TestResultFilter`` takes a new optional constructor parameter | ||
9 | 34 | ``filter_predicate``. (Martin Pool) | ||
10 | 35 | |||
11 | 33 | BUG FIXES: | 36 | BUG FIXES: |
12 | 34 | 37 | ||
13 | 35 | API CHANGES: | 38 | API CHANGES: |
14 | 36 | 39 | ||
15 | === modified file 'README' | |||
16 | --- README 2009-08-01 08:01:27 +0000 | |||
17 | +++ README 2009-08-05 07:22:36 +0000 | |||
18 | @@ -169,6 +169,13 @@ | |||
19 | 169 | elements, and a patch for adding subunit output to the 'ShUnit' shell test | 169 | elements, and a patch for adding subunit output to the 'ShUnit' shell test |
20 | 170 | runner. See 'shell/README' for details. | 170 | runner. See 'shell/README' for details. |
21 | 171 | 171 | ||
22 | 172 | Filter recipes | ||
23 | 173 | -------------- | ||
24 | 174 | |||
25 | 175 | To ignore some failing tests whose root cause is already known:: | ||
26 | 176 | |||
27 | 177 | subunit-filter --anti-match 'AttributeError.*flavor' | ||
28 | 178 | |||
29 | 172 | 179 | ||
30 | 173 | The protocol | 180 | The protocol |
31 | 174 | ------------ | 181 | ------------ |
32 | 175 | 182 | ||
33 | === modified file 'filters/subunit-filter' | |||
34 | --- filters/subunit-filter 2009-08-05 05:21:50 +0000 | |||
35 | +++ filters/subunit-filter 2009-08-05 07:21:05 +0000 | |||
36 | @@ -1,6 +1,7 @@ | |||
37 | 1 | #!/usr/bin/env python | 1 | #!/usr/bin/env python |
38 | 2 | # subunit: extensions to python unittest to get test results from subprocesses. | 2 | # subunit: extensions to python unittest to get test results from subprocesses. |
39 | 3 | # Copyright (C) 2008 Robert Collins <robertc@robertcollins.net> | 3 | # Copyright (C) 2008 Robert Collins <robertc@robertcollins.net> |
40 | 4 | # (C) 2009 Martin Pool | ||
41 | 4 | # | 5 | # |
42 | 5 | # This program is free software; you can redistribute it and/or modify | 6 | # This program is free software; you can redistribute it and/or modify |
43 | 6 | # it under the terms of the GNU General Public License as published by | 7 | # it under the terms of the GNU General Public License as published by |
44 | @@ -25,6 +26,7 @@ | |||
45 | 25 | from optparse import OptionParser | 26 | from optparse import OptionParser |
46 | 26 | import sys | 27 | import sys |
47 | 27 | import unittest | 28 | import unittest |
48 | 29 | import re | ||
49 | 28 | 30 | ||
50 | 29 | from subunit import ( | 31 | from subunit import ( |
51 | 30 | DiscardStream, | 32 | DiscardStream, |
52 | @@ -50,11 +52,37 @@ | |||
53 | 50 | help="exclude skips", dest="skip") | 52 | help="exclude skips", dest="skip") |
54 | 51 | parser.add_option("--no-success", action="store_true", | 53 | parser.add_option("--no-success", action="store_true", |
55 | 52 | help="exclude successes", default=True, dest="success") | 54 | help="exclude successes", default=True, dest="success") |
56 | 55 | parser.add_option("-m", "--match", type=str, | ||
57 | 56 | help="regexp to include (case-sensitive by default)") | ||
58 | 57 | parser.add_option("--anti-match", type=str, | ||
59 | 58 | help="regexp to exclude (case-sensitive by default)") | ||
60 | 59 | |||
61 | 53 | (options, args) = parser.parse_args() | 60 | (options, args) = parser.parse_args() |
62 | 61 | |||
63 | 62 | def make_re_matcher(needle_re_string): | ||
64 | 63 | needle_re = re.compile(needle_re_string, re.MULTILINE) | ||
65 | 64 | return lambda test, err: ( | ||
66 | 65 | needle_re.search(str(test)) or | ||
67 | 66 | ((err is not None) and needle_re.search(str(err)))) | ||
68 | 67 | |||
69 | 68 | if options.match: | ||
70 | 69 | match_predicate = make_re_matcher(options.match) | ||
71 | 70 | else: | ||
72 | 71 | match_predicate = lambda test, err: True | ||
73 | 72 | |||
74 | 73 | if options.anti_match: | ||
75 | 74 | anti_match_predicate = make_re_matcher(options.anti_match) | ||
76 | 75 | else: | ||
77 | 76 | anti_match_predicate = lambda test, err: False | ||
78 | 77 | |||
79 | 78 | filter_predicate = lambda test, err: ( | ||
80 | 79 | match_predicate(test, err) and not anti_match_predicate(test, err)) | ||
81 | 80 | |||
82 | 54 | result = TestProtocolClient(sys.stdout) | 81 | result = TestProtocolClient(sys.stdout) |
83 | 55 | result = TestResultFilter(result, filter_error=options.error, | 82 | result = TestResultFilter(result, filter_error=options.error, |
84 | 56 | filter_failure=options.failure, filter_success=options.success, | 83 | filter_failure=options.failure, filter_success=options.success, |
86 | 57 | filter_skip=options.skip) | 84 | filter_skip=options.skip, |
87 | 85 | filter_predicate=filter_predicate) | ||
88 | 58 | if options.no_passthrough: | 86 | if options.no_passthrough: |
89 | 59 | passthrough_stream = DiscardStream() | 87 | passthrough_stream = DiscardStream() |
90 | 60 | else: | 88 | else: |
91 | 61 | 89 | ||
92 | === modified file 'python/subunit/__init__.py' | |||
93 | --- python/subunit/__init__.py 2009-08-02 22:55:36 +0000 | |||
94 | +++ python/subunit/__init__.py 2009-08-05 06:57:48 +0000 | |||
95 | @@ -792,16 +792,22 @@ | |||
96 | 792 | the other instance must be interrogated. | 792 | the other instance must be interrogated. |
97 | 793 | 793 | ||
98 | 794 | :ivar result: The result that tests are passed to after filtering. | 794 | :ivar result: The result that tests are passed to after filtering. |
99 | 795 | :ivar filter_predicate: The callback run to decide whether to pass | ||
100 | 796 | a result. | ||
101 | 795 | """ | 797 | """ |
102 | 796 | 798 | ||
103 | 797 | def __init__(self, result, filter_error=False, filter_failure=False, | 799 | def __init__(self, result, filter_error=False, filter_failure=False, |
105 | 798 | filter_success=True, filter_skip=False): | 800 | filter_success=True, filter_skip=False, |
106 | 801 | filter_predicate=None): | ||
107 | 799 | """Create a FilterResult object filtering to result. | 802 | """Create a FilterResult object filtering to result. |
108 | 800 | 803 | ||
109 | 801 | :param filter_error: Filter out errors. | 804 | :param filter_error: Filter out errors. |
110 | 802 | :param filter_failure: Filter out failures. | 805 | :param filter_failure: Filter out failures. |
111 | 803 | :param filter_success: Filter out successful tests. | 806 | :param filter_success: Filter out successful tests. |
112 | 804 | :param filter_skip: Filter out skipped tests. | 807 | :param filter_skip: Filter out skipped tests. |
113 | 808 | :param filter_predicate: A callable taking (test, err) and | ||
114 | 809 | returning True if the result should be passed through. | ||
115 | 810 | err is None for success. | ||
116 | 805 | """ | 811 | """ |
117 | 806 | unittest.TestResult.__init__(self) | 812 | unittest.TestResult.__init__(self) |
118 | 807 | self.result = result | 813 | self.result = result |
119 | @@ -809,21 +815,24 @@ | |||
120 | 809 | self._filter_failure = filter_failure | 815 | self._filter_failure = filter_failure |
121 | 810 | self._filter_success = filter_success | 816 | self._filter_success = filter_success |
122 | 811 | self._filter_skip = filter_skip | 817 | self._filter_skip = filter_skip |
123 | 818 | if filter_predicate is None: | ||
124 | 819 | filter_predicate = lambda test, err: True | ||
125 | 820 | self.filter_predicate = filter_predicate | ||
126 | 812 | 821 | ||
127 | 813 | def addError(self, test, err): | 822 | def addError(self, test, err): |
129 | 814 | if not self._filter_error: | 823 | if not self._filter_error and self.filter_predicate(test, err): |
130 | 815 | self.result.startTest(test) | 824 | self.result.startTest(test) |
131 | 816 | self.result.addError(test, err) | 825 | self.result.addError(test, err) |
132 | 817 | self.result.stopTest(test) | 826 | self.result.stopTest(test) |
133 | 818 | 827 | ||
134 | 819 | def addFailure(self, test, err): | 828 | def addFailure(self, test, err): |
136 | 820 | if not self._filter_failure: | 829 | if not self._filter_failure and self.filter_predicate(test, err): |
137 | 821 | self.result.startTest(test) | 830 | self.result.startTest(test) |
138 | 822 | self.result.addFailure(test, err) | 831 | self.result.addFailure(test, err) |
139 | 823 | self.result.stopTest(test) | 832 | self.result.stopTest(test) |
140 | 824 | 833 | ||
141 | 825 | def addSkip(self, test, reason): | 834 | def addSkip(self, test, reason): |
143 | 826 | if not self._filter_skip: | 835 | if not self._filter_skip and self.filter_predicate(test, reason): |
144 | 827 | self.result.startTest(test) | 836 | self.result.startTest(test) |
145 | 828 | # This is duplicated, it would be nice to have on a 'calls | 837 | # This is duplicated, it would be nice to have on a 'calls |
146 | 829 | # TestResults' mixin perhaps. | 838 | # TestResults' mixin perhaps. |
147 | @@ -835,7 +844,7 @@ | |||
148 | 835 | self.result.stopTest(test) | 844 | self.result.stopTest(test) |
149 | 836 | 845 | ||
150 | 837 | def addSuccess(self, test): | 846 | def addSuccess(self, test): |
152 | 838 | if not self._filter_success: | 847 | if not self._filter_success and self.filter_predicate(test, None): |
153 | 839 | self.result.startTest(test) | 848 | self.result.startTest(test) |
154 | 840 | self.result.addSuccess(test) | 849 | self.result.addSuccess(test) |
155 | 841 | self.result.stopTest(test) | 850 | self.result.stopTest(test) |
156 | 842 | 851 | ||
157 | === modified file 'python/subunit/tests/test_subunit_filter.py' | |||
158 | --- python/subunit/tests/test_subunit_filter.py 2009-03-08 20:34:19 +0000 | |||
159 | +++ python/subunit/tests/test_subunit_filter.py 2009-08-05 06:57:48 +0000 | |||
160 | @@ -91,6 +91,19 @@ | |||
161 | 91 | self.filtered_result.failures]) | 91 | self.filtered_result.failures]) |
162 | 92 | self.assertEqual(5, self.filtered_result.testsRun) | 92 | self.assertEqual(5, self.filtered_result.testsRun) |
163 | 93 | 93 | ||
164 | 94 | def test_filter_predicate(self): | ||
165 | 95 | """You can filter by predicate callbacks""" | ||
166 | 96 | self.filtered_result = unittest.TestResult() | ||
167 | 97 | filter_cb = lambda test, err: str(err).find('error details') != -1 | ||
168 | 98 | self.filter = subunit.TestResultFilter(self.filtered_result, | ||
169 | 99 | filter_predicate=filter_cb, | ||
170 | 100 | filter_success=False) | ||
171 | 101 | self.run_tests() | ||
172 | 102 | self.assertEqual(1, | ||
173 | 103 | self.filtered_result.testsRun) | ||
174 | 104 | # I'd like to test filtering the xfail but it's blocked by | ||
175 | 105 | # https://bugs.edge.launchpad.net/subunit/+bug/409193 -- mbp 20090805 | ||
176 | 106 | |||
177 | 94 | def run_tests(self): | 107 | def run_tests(self): |
178 | 95 | self.setUpTestStream() | 108 | self.setUpTestStream() |
179 | 96 | self.test = subunit.ProtocolTestCase(self.input_stream) | 109 | self.test = subunit.ProtocolTestCase(self.input_stream) |
180 | @@ -109,7 +122,9 @@ | |||
181 | 109 | tags: local | 122 | tags: local |
182 | 110 | failure failed | 123 | failure failed |
183 | 111 | test error | 124 | test error |
185 | 112 | error error | 125 | error error [ |
186 | 126 | error details | ||
187 | 127 | ] | ||
188 | 113 | test skipped | 128 | test skipped |
189 | 114 | skip skipped | 129 | skip skipped |
190 | 115 | test todo | 130 | test todo |
Tested interactively, not yet tested anger, but I anticipate it being very useful.