Merge lp:~gmb/zope.testing/check-for-unique-tests into lp:~launchpad/zope.testing/3.9.4-fork

Proposed by Graham Binns on 2012-06-14
Status: Merged
Merged at revision: 40
Proposed branch: lp:~gmb/zope.testing/check-for-unique-tests
Merge into: lp:~launchpad/zope.testing/3.9.4-fork
Diff against target: 260 lines (+152/-6)
5 files modified
setup.py (+1/-1)
src/zope/testing/testrunner/find.py (+38/-4)
src/zope/testing/testrunner/options.py (+16/-0)
src/zope/testing/testrunner/test_uniqueness.py (+95/-0)
src/zope/testing/testrunner/tests.py (+2/-1)
To merge this branch: bzr merge lp:~gmb/zope.testing/check-for-unique-tests
Reviewer Review Type Date Requested Status
Brad Crittenden (community) code 2012-06-14 Approve on 2012-06-14
Review via email: mp+110262@code.launchpad.net

Description of the change

This branch fixes bug 682771 by introducing a --require-unique flag to zope.testing's testrunner. When passed, this flag causes the testrunner to check every test ID it loads to see if it's seen it before. If it has, it raises an error and exits.

I wanted to make this the default behaviour, but that would have caused zope.testing's own test suite to break, as there are places where it will have duplicate test IDs (especially when testing layers).

This will probably be p13 of our zope.testing fork; I'll update the version number once the branch is approved for merging. (But hey, prime number! Therefore it will break nothing, right?)

To post a comment you must log in.
Brad Crittenden (bac) wrote :

Hi Graham, thanks for this change, it'll help ensure we don't go down the rabbit hole of duplicate tests again.

* On IRC we discussed finding and reporting all duplicates rather than bailing on the first. I think that'll make it much more usable.

* The version number should be bumped to -p13.

* Using a set for seen_test_ids will be much faster since set membership is O(1) thanks to the use of hashing. Testing list membership probably isn't as bad as O(n) but it is much slower.

* --require-unique does not work with some other filtering options such as '-m'. Using r15410 of trunk I did the following:

% bin/test --require-unique --list-tests -m lp.code

Produces no warnings.

% bin/test --require-unique --list-tests -m lp.answers

Does find the duplicate faqcollection.txt.

So perhaps a warning should be issued when using other options that will provide incomplete results.

Thanks again for such a nice implementation. I'm happy it was so minimal.

review: Approve (code)
40. By Graham Binns on 2012-06-14

Added a cumulative error handling, + tests and other assorted reviewer-requested changes.

41. By Graham Binns on 2012-06-14

Bumped version to -p13.

42. By Graham Binns on 2012-06-14

Merged parent.

43. By Graham Binns on 2012-06-14

Added a warning for -m and --require-unique being used together.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'setup.py'
2--- setup.py 2012-06-13 19:22:47 +0000
3+++ setup.py 2012-06-14 17:30:24 +0000
4@@ -85,7 +85,7 @@
5
6 setup(
7 name='zope.testing',
8- version = '3.9.4-p12',
9+ version = '3.9.4-p13',
10 url='http://pypi.python.org/pypi/zope.testing',
11 license='ZPL 2.1',
12 description='Zope testing framework, including the testrunner script.',
13
14=== modified file 'src/zope/testing/testrunner/find.py'
15--- src/zope/testing/testrunner/find.py 2012-06-06 22:57:14 +0000
16+++ src/zope/testing/testrunner/find.py 2012-06-14 17:30:24 +0000
17@@ -28,6 +28,10 @@
18 identifier = re.compile(r'[_a-zA-Z]\w*$').match
19
20
21+class DuplicateTestIDError(Exception):
22+ """Raised whenever a test ID is encountered twice during loading."""
23+
24+
25 class StartUpFailure(unittest.TestCase):
26 """Empty test case added to the test suite to indicate import failures.
27
28@@ -112,14 +116,27 @@
29 """
30 remove_stale_bytecode(options)
31 suites = {}
32+ dupe_ids = set()
33 if found_suites is None:
34 found_suites = find_suites(options)
35 for suite in found_suites:
36- for test, layer_name in tests_from_suite(suite, options):
37+ tests_and_layers = tests_from_suite(
38+ suite, options, duplicated_test_ids=dupe_ids)
39+ for test, layer_name in tests_and_layers:
40+ if len(dupe_ids) > 0:
41+ # If there are any duplicated test IDs, we stop trying
42+ # to load tests; we'll raise an error later on with all
43+ # the duplicates in it.
44+ continue
45 suite = suites.get(layer_name)
46 if not suite:
47 suite = suites[layer_name] = unittest.TestSuite()
48 suite.addTest(test)
49+ if len(dupe_ids) > 0:
50+ message_lines = [
51+ "Duplicate test IDs found:"] + sorted(dupe_ids)
52+ message = "\n ".join(message_lines)
53+ raise DuplicateTestIDError(message)
54 return suites
55
56
57@@ -335,9 +352,9 @@
58 __import__(name)
59 return sys.modules[name]
60
61-
62 def tests_from_suite(suite, options, dlevel=1,
63- dlayer=zope.testing.testrunner.layer.UnitTests):
64+ dlayer=zope.testing.testrunner.layer.UnitTests,
65+ seen_test_ids=None, duplicated_test_ids=None):
66 """Returns a sequence of (test, layer_name)
67
68 The tree of suites is recursively visited, with the most specific
69@@ -348,6 +365,14 @@
70 Tests are also filtered out based on the test level and test selection
71 filters stored in the options.
72 """
73+ # We use this to track the test IDs that have been registered.
74+ # tests_from_suite() will complain if it encounters the same test ID
75+ # twice.
76+ if seen_test_ids is None:
77+ seen_test_ids = set()
78+ if duplicated_test_ids is None:
79+ duplicated_test_ids = set()
80+
81 level = getattr(suite, 'level', dlevel)
82 layer = getattr(suite, 'layer', dlayer)
83 if not isinstance(layer, basestring):
84@@ -355,11 +380,20 @@
85
86 if isinstance(suite, unittest.TestSuite):
87 for possible_suite in suite:
88- for r in tests_from_suite(possible_suite, options, level, layer):
89+ from_suite = tests_from_suite(
90+ possible_suite, options, level, layer, seen_test_ids,
91+ duplicated_test_ids)
92+ for r in from_suite:
93 yield r
94 elif isinstance(suite, StartUpFailure):
95 yield (suite, None)
96 else:
97+ if options.require_unique_ids:
98+ suite_id = suite.id()
99+ if suite_id in seen_test_ids:
100+ duplicated_test_ids.add(suite_id)
101+ else:
102+ seen_test_ids.add(suite_id)
103 if level <= options.at_level:
104 for pat in options.test:
105 if pat(str(suite)):
106
107=== modified file 'src/zope/testing/testrunner/options.py'
108--- src/zope/testing/testrunner/options.py 2012-06-13 19:22:47 +0000
109+++ src/zope/testing/testrunner/options.py 2012-06-14 17:30:24 +0000
110@@ -131,6 +131,13 @@
111 '--list-tests', action="store_true", dest='list_tests',
112 help="List all tests that matched your filters. Do not run any tests.")
113
114+searching.add_option(
115+ '--require-unique', action="store_true", dest='require_unique_ids',
116+ help="""\
117+Require that all test IDs be unique and raise an error if duplicates are
118+encountered.
119+""")
120+
121 parser.add_option_group(searching)
122
123 ######################################################################
124@@ -695,6 +702,15 @@
125 options.fail = True
126 return options
127
128+ if options.module and options.require_unique_ids:
129+ # We warn if --module and --require-unique are specified at the
130+ # same time, though we don't exit.
131+ print """\
132+ You specified a module along with --require-unique;
133+ --require-unique will not try to enforce test ID uniqueness when
134+ working with a specific module.
135+ """
136+
137 return options
138
139 def normalize_package(package, package_map={}):
140
141=== added file 'src/zope/testing/testrunner/test_uniqueness.py'
142--- src/zope/testing/testrunner/test_uniqueness.py 1970-01-01 00:00:00 +0000
143+++ src/zope/testing/testrunner/test_uniqueness.py 2012-06-14 17:30:24 +0000
144@@ -0,0 +1,95 @@
145+##############################################################################
146+#
147+# Copyright (c) 2004-2012 Zope Foundation and Contributors.
148+# All Rights Reserved.
149+#
150+# This software is subject to the provisions of the Zope Public License,
151+# Version 2.1 (ZPL). A copy of the ZPL should accompany this distribution.
152+# THIS SOFTWARE IS PROVIDED "AS IS" AND ANY AND ALL EXPRESS OR IMPLIED
153+# WARRANTIES ARE DISCLAIMED, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
154+# WARRANTIES OF TITLE, MERCHANTABILITY, AGAINST INFRINGEMENT, AND FITNESS
155+# FOR A PARTICULAR PURPOSE.
156+#
157+##############################################################################
158+"""Unit tests for the testrunner's handling of non-unique test IDs."""
159+
160+
161+import unittest
162+
163+from zope.testing import doctest
164+from zope.testing.testrunner.find import (
165+ DuplicateTestIDError,
166+ find_tests,
167+ tests_from_suite,
168+ )
169+
170+
171+class Options:
172+ """A basic mock of our command-line options."""
173+ keepbytecode = True
174+ at_level = 99
175+ test = []
176+ require_unique_ids = True
177+
178+
179+class UniquenessTestCase(unittest.TestCase):
180+ """A test case to test how the testrunner handles non-unique IDs."""
181+
182+ def setUp(self):
183+ super(UniquenessTestCase, self).setUp()
184+ suites = [
185+ doctest.DocFileSuite('testrunner-ex/sampletests.txt'),
186+ doctest.DocFileSuite('testrunner-ex/sampletests.txt'),
187+ doctest.DocFileSuite('testrunner-ex/sampletestsl.txt'),
188+ doctest.DocFileSuite('testrunner-ex/sampletestsl.txt'),
189+ ]
190+ self.test_suites = unittest.TestSuite(suites)
191+
192+ def _get_duplicate_tests(self, options):
193+ # We have to wrap this in a function as tests_from_suite()
194+ # returns a generator, so the error won't be triggered until we
195+ # actually try to iterate over the results.
196+ seen_test_ids = set()
197+ duplicated_test_ids = set()
198+ tests = [
199+ test for test in tests_from_suite(
200+ self.test_suites, options, seen_test_ids=seen_test_ids,
201+ duplicated_test_ids=duplicated_test_ids)]
202+ return duplicated_test_ids
203+
204+ def test_tests_from_suite_records_duplicate_test_ids(self):
205+ """If tests_from_suite() encounters a test ID which has already
206+ been registered, it will record them in the duplicated_test_ids
207+ set it is passed.
208+ """
209+ duplicated_test_ids = self._get_duplicate_tests(
210+ Options())
211+ self.assertNotEqual(0, len(duplicated_test_ids))
212+
213+ def test_tests_from_suite_ignores_duplicate_ids_if_option_not_set(self):
214+ """If the require_unique_ids flag is not set, tests_from_suite()
215+ will not record any of the IDs as duplicates.
216+ """
217+ options = Options()
218+ options.require_unique_ids = False
219+ duplicated_test_ids = self._get_duplicate_tests(
220+ options)
221+ self.assertEqual(0, len(duplicated_test_ids))
222+
223+ def test_find_tests_raises_error_if_duplicates_found(self):
224+ """If find tests, which calls tests_from_suite, finds a
225+ duplicate test ID, it raises an error.
226+ """
227+ self.assertRaises(
228+ DuplicateTestIDError, find_tests, Options(), [self.test_suites])
229+
230+ def test_DuplicateTestIDError_message_contains_all_test_ids(self):
231+ """The error raised by find_tests when duplicates are
232+ encountered contains all the duplicate test IDs it found.
233+ """
234+ try:
235+ find_tests(Options(), [self.test_suites])
236+ except DuplicateTestIDError, exception:
237+ pass
238+ self.assertIn('sampletests_txt', exception.message)
239+ self.assertIn('sampletestsl_txt', exception.message)
240
241=== modified file 'src/zope/testing/testrunner/tests.py'
242--- src/zope/testing/testrunner/tests.py 2012-03-26 15:13:24 +0000
243+++ src/zope/testing/testrunner/tests.py 2012-06-14 17:30:24 +0000
244@@ -194,6 +194,8 @@
245 optionflags=doctest.ELLIPSIS+doctest.NORMALIZE_WHITESPACE),
246 doctest.DocTestSuite('zope.testing.testrunner.options'),
247 doctest.DocTestSuite('zope.testing.testrunner.find'),
248+ unittest.defaultTestLoader.loadTestsFromName(
249+ 'zope.testing.testrunner.test_uniqueness'),
250 ]
251
252 if sys.platform == 'win32':
253@@ -303,7 +305,6 @@
254 setUp=setUp, tearDown=tearDown,
255 optionflags=doctest.ELLIPSIS + doctest.NORMALIZE_WHITESPACE,
256 checker=checker))
257-
258 suites.append(
259 unittest.defaultTestLoader.loadTestsFromName(
260 'zope.testing.testrunner.test_subunit'))

Subscribers

People subscribed via source and target branches