Bazaar

Merge lp://staging/~mbp/bzr/selftest into lp://staging/~bzr/bzr/trunk-old

  1. selftest
  2. Merge into trunk-old
Proposed by Martin Pool
Status: Merged
Merged at revision: not available
Proposed branch: lp://staging/~mbp/bzr/selftest
Merge into: lp://staging/~bzr/bzr/trunk-old
Diff against target: 300 lines
To merge this branch: bzr merge lp://staging/~mbp/bzr/selftest
Reviewer Review Type Date Requested Status
Robert Collins (community) Approve
Review via email: mp+9571@code.staging.launchpad.net
To post a comment you must log in.
Revision history for this message
Martin Pool (mbp) wrote :

Fixes some annoying glitches in selftest reporting of progress:

--- NEWS 2009-07-31 16:22:11 +0000
+++ NEWS 2009-08-03 05:31:00 +0000
@@ -30,6 +30,11 @@
   There are still some complex scenarios where it will fail (bug #399884)
   (John Arbash Meinel, #393366)

+* A progress bar is no longer left dangling when ``bzr selftest``
+ completes, and the progress bar updates with zero latency so the
+ displayed test name is always the one that's actually running.
+ (Martin Pool, #123688)
+
 * Authenticating against an ssh server now uses ``auth_none`` to determine
   if password authentication is even supported. This fixes a bug where
   users would be prompted for a launchpad password, even though launchpad
@@ -77,6 +82,9 @@
 * Fixed spurious "Source branch does not support stacking" warning when
   pushing. (Andrew Bennetts, #388908)

+* Fixed spurious transport activity indicator appearing while tests are
+ running. (Martin Pool, #343532)
+
 * Merge now correctly handles empty right-hand revision specs.
   (Aaron Bentley, #333961)

@@ -139,6 +147,9 @@
 * New TransformPreview.commit() allows committing without a working tree.
   (Aaron Bentley)

+* ``pb`` parameter to ``TextTestResult`` is deprecated and ignored.
+ (Martin Pool)
+
 * ProgressTasks now prefer to talk direct to their ProgressView not to the
   UIFactory.
   (Martin Pool)

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

Looks good to me

review: Approve
Revision history for this message
John A Meinel (jameinel) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin Pool wrote:
> Martin Pool has proposed merging lp:~mbp/bzr/selftest into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> Fixes some annoying glitches in selftest reporting of progress:
>

So as for annoying bits the only one that I see missing is that my
selftest progress bar seems to show transport activity from time to time.

All the changes you've made here are positive, though. Just one more I'd
like to point out while you're working in the area.

(Even worse, it almost always shows 0KB/s / 0KB, but it still throws off
the formatting.)

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkp251wACgkQJdeBCYSNAAMMygCguSV3Vht9Dk/ucdb64hJ6hwQL
hIMAn19TOr7CnSaZ2J9EScLLmezI1EXK
=osyL
-----END PGP SIGNATURE-----

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

2009/8/3 John A Meinel <email address hidden>:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Martin Pool wrote:
>> Martin Pool has proposed merging lp:~mbp/bzr/selftest into lp:bzr.
>>
>> Requested reviews:
>>     bzr-core (bzr-core)
>>
>> Fixes some annoying glitches in selftest reporting of progress:
>>
>
> So as for annoying bits the only one that I see missing is that my
> selftest progress bar seems to show transport activity from time to time.
>
> All the changes you've made here are positive, though. Just one more I'd
> like to point out while you're working in the area.
>
> (Even worse, it almost always shows 0KB/s / 0KB, but it still throws off
> the formatting.)

Right, that was

+* Fixed spurious transport activity indicator appearing while tests are
+ running. (Martin Pool, #343532)

The other glitches were:

 * the test shown in the progress bar was not guaranteed to be the one
that's actually running at the time; when you have a mix of slow and
fast tests as we commonly do this means you don't get a clear
indication where it's spending its time

 * the pb is left behind on the screen at the end

--
Martin <http://launchpad.net/~mbp/>

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-08-04 00:55:07 +0000
3+++ NEWS 2009-08-04 04:35:12 +0000
4@@ -30,6 +30,11 @@
5 There are still some complex scenarios where it will fail (bug #399884)
6 (John Arbash Meinel, #393366)
7
8+* A progress bar is no longer left dangling when ``bzr selftest``
9+ completes, and the progress bar updates with zero latency so the
10+ displayed test name is always the one that's actually running.
11+ (Martin Pool, #123688)
12+
13 * Authenticating against an ssh server now uses ``auth_none`` to determine
14 if password authentication is even supported. This fixes a bug where
15 users would be prompted for a launchpad password, even though launchpad
16@@ -74,6 +79,9 @@
17 * Fixed spurious "Source branch does not support stacking" warning when
18 pushing. (Andrew Bennetts, #388908)
19
20+* Fixed spurious transport activity indicator appearing while tests are
21+ running. (Martin Pool, #343532)
22+
23 * Merge now correctly handles empty right-hand revision specs.
24 (Aaron Bentley, #333961)
25
26@@ -143,6 +151,9 @@
27 * New TransformPreview.commit() allows committing without a working tree.
28 (Aaron Bentley)
29
30+* ``pb`` parameter to ``TextTestResult`` is deprecated and ignored.
31+ (Martin Pool)
32+
33 * ProgressTasks now prefer to talk direct to their ProgressView not to the
34 UIFactory.
35 (Martin Pool)
36
37=== modified file 'bzrlib/progress.py'
38--- bzrlib/progress.py 2009-07-17 10:38:41 +0000
39+++ bzrlib/progress.py 2009-08-04 04:35:12 +0000
40@@ -69,6 +69,15 @@
41 Code updating the task may also set fields as hints about how to display
42 it: show_pct, show_spinner, show_eta, show_count, show_bar. UIs
43 will not necessarily respect all these fields.
44+
45+ :ivar update_latency: The interval (in seconds) at which the PB should be
46+ updated. Setting this to zero suggests every update should be shown
47+ synchronously.
48+
49+ :ivar show_transport_activity: If true (default), transport activity
50+ will be shown when this task is drawn. Disable it if you're sure
51+ that only irrelevant or uninteresting transport activity can occur
52+ during this task.
53 """
54
55 def __init__(self, parent_task=None, ui_factory=None, progress_view=None):
56@@ -97,6 +106,8 @@
57 self.show_eta = False,
58 self.show_count = True
59 self.show_bar = True
60+ self.update_latency = 0.1
61+ self.show_transport_activity = True
62
63 def __repr__(self):
64 return '%s(%r/%r, msg=%r)' % (
65
66=== modified file 'bzrlib/tests/__init__.py'
67--- bzrlib/tests/__init__.py 2009-08-04 00:55:07 +0000
68+++ bzrlib/tests/__init__.py 2009-08-04 04:35:12 +0000
69@@ -175,6 +175,8 @@
70 self._strict = strict
71
72 def done(self):
73+ # nb: called stopTestRun in the version of this that Python merged
74+ # upstream, according to lifeless 20090803
75 if self._strict:
76 ok = self.wasStrictlySuccessful()
77 else:
78@@ -397,21 +399,35 @@
79 ):
80 ExtendedTestResult.__init__(self, stream, descriptions, verbosity,
81 bench_history, strict)
82- if pb is None:
83- self.pb = self.ui.nested_progress_bar()
84- self._supplied_pb = False
85- else:
86- self.pb = pb
87- self._supplied_pb = True
88+ # We no longer pass them around, but just rely on the UIFactory stack
89+ # for state
90+ if pb is not None:
91+ warnings.warn("Passing pb to TextTestResult is deprecated")
92+ self.pb = self.ui.nested_progress_bar()
93 self.pb.show_pct = False
94 self.pb.show_spinner = False
95 self.pb.show_eta = False,
96 self.pb.show_count = False
97 self.pb.show_bar = False
98+ self.pb.update_latency = 0
99+ self.pb.show_transport_activity = False
100+
101+ def done(self):
102+ # called when the tests that are going to run have run
103+ self.pb.clear()
104+ super(TextTestResult, self).done()
105+
106+ def finished(self):
107+ self.pb.finished()
108
109 def report_starting(self):
110 self.pb.update('[test 0/%d] Starting' % (self.num_tests))
111
112+ def printErrors(self):
113+ # clear the pb to make room for the error listing
114+ self.pb.clear()
115+ super(TextTestResult, self).printErrors()
116+
117 def _progress_prefix_text(self):
118 # the longer this text, the less space we have to show the test
119 # name...
120@@ -477,10 +493,6 @@
121 def report_cleaning_up(self):
122 self.pb.update('Cleaning up')
123
124- def finished(self):
125- if not self._supplied_pb:
126- self.pb.finished()
127-
128
129 class VerboseTestResult(ExtendedTestResult):
130 """Produce long output, with one line per test run plus times"""
131@@ -724,6 +736,9 @@
132 See also CannedInputUIFactory which lets you provide programmatic input in
133 a structured way.
134 """
135+ # TODO: Capture progress events at the model level and allow them to be
136+ # observed by tests that care.
137+ #
138 # XXX: Should probably unify more with CannedInputUIFactory or a
139 # particular configuration of TextUIFactory, or otherwise have a clearer
140 # idea of how they're supposed to be different.
141
142=== modified file 'bzrlib/tests/test_selftest.py'
143--- bzrlib/tests/test_selftest.py 2009-08-03 01:57:07 +0000
144+++ bzrlib/tests/test_selftest.py 2009-08-04 04:35:13 +0000
145@@ -681,29 +681,6 @@
146 self.assertEqual(url, t.clone('..').base)
147
148
149-class MockProgress(progress._BaseProgressBar):
150- """Progress-bar standin that records calls.
151-
152- Useful for testing pb using code.
153- """
154-
155- def __init__(self):
156- progress._BaseProgressBar.__init__(self)
157- self.calls = []
158-
159- def tick(self):
160- self.calls.append(('tick',))
161-
162- def update(self, msg=None, current=None, total=None):
163- self.calls.append(('update', msg, current, total))
164-
165- def clear(self):
166- self.calls.append(('clear',))
167-
168- def note(self, msg, *args):
169- self.calls.append(('note', msg, args))
170-
171-
172 class TestTestResult(tests.TestCase):
173
174 def check_timing(self, test_case, expected_re):
175@@ -862,41 +839,6 @@
176 self.assertEqual(lines[1], ' foo')
177 self.assertEqual(2, len(lines))
178
179- def test_text_report_known_failure(self):
180- # text test output formatting
181- pb = MockProgress()
182- result = bzrlib.tests.TextTestResult(
183- StringIO(),
184- descriptions=0,
185- verbosity=1,
186- pb=pb,
187- )
188- test = self.get_passing_test()
189- # this seeds the state to handle reporting the test.
190- result.startTest(test)
191- # the err parameter has the shape:
192- # (class, exception object, traceback)
193- # KnownFailures dont get their tracebacks shown though, so we
194- # can skip that.
195- err = (tests.KnownFailure, tests.KnownFailure('foo'), None)
196- result.report_known_failure(test, err)
197- self.assertEqual(
198- [
199- ('update', '[1 in 0s] passing_test', None, None),
200- ('note', 'XFAIL: %s\n%s\n', ('passing_test', err[1]))
201- ],
202- pb.calls)
203- # known_failures should be printed in the summary, so if we run a test
204- # after there are some known failures, the update prefix should match
205- # this.
206- result.known_failure_count = 3
207- test.run(result)
208- self.assertEqual(
209- [
210- ('update', '[2 in 0s] passing_test', None, None),
211- ],
212- pb.calls[2:])
213-
214 def get_passing_test(self):
215 """Return a test object that can't be run usefully."""
216 def passing_test():
217@@ -947,35 +889,6 @@
218 self.assertEqual(lines, ['NODEP 0ms',
219 " The feature 'Feature' is not available."])
220
221- def test_text_report_unsupported(self):
222- # text test output formatting
223- pb = MockProgress()
224- result = bzrlib.tests.TextTestResult(
225- StringIO(),
226- descriptions=0,
227- verbosity=1,
228- pb=pb,
229- )
230- test = self.get_passing_test()
231- feature = tests.Feature()
232- # this seeds the state to handle reporting the test.
233- result.startTest(test)
234- result.report_unsupported(test, feature)
235- # no output on unsupported features
236- self.assertEqual(
237- [('update', '[1 in 0s] passing_test', None, None)
238- ],
239- pb.calls)
240- # the number of missing features should be printed in the progress
241- # summary, so check for that.
242- result.unsupported = {'foo':0, 'bar':0}
243- test.run(result)
244- self.assertEqual(
245- [
246- ('update', '[2 in 0s, 2 missing] passing_test', None, None),
247- ],
248- pb.calls[1:])
249-
250 def test_unavailable_exception(self):
251 """An UnavailableFeature being raised should invoke addNotSupported."""
252 class InstrumentedTestResult(tests.ExtendedTestResult):
253
254=== modified file 'bzrlib/ui/text.py'
255--- bzrlib/ui/text.py 2009-07-24 16:36:51 +0000
256+++ bzrlib/ui/text.py 2009-08-04 04:35:13 +0000
257@@ -224,6 +224,7 @@
258 self._bytes_since_update = 0
259
260 def _show_line(self, s):
261+ # sys.stderr.write("progress %r\n" % s)
262 n = self._width - 1
263 self._term_file.write('\r%-*.*s\r' % (n, n, s))
264
265@@ -283,9 +284,12 @@
266 task_msg = self._format_task(self._last_task)
267 else:
268 task_msg = ''
269- trans = self._last_transport_msg
270- if trans:
271- trans += ' | '
272+ if self._last_task and not self._last_task.show_transport_activity:
273+ trans = ''
274+ else:
275+ trans = self._last_transport_msg
276+ if trans:
277+ trans += ' | '
278 return (bar_string + trans + task_msg)
279
280 def _repaint(self):
281@@ -302,7 +306,7 @@
282 must_update = task is not self._last_task
283 self._last_task = task
284 now = time.time()
285- if (not must_update) and (now < self._last_repaint + 0.1):
286+ if (not must_update) and (now < self._last_repaint + task.update_latency):
287 return
288 if now > self._transport_update_time + 10:
289 # no recent activity; expire it
290@@ -330,6 +334,10 @@
291 self._total_byte_count += byte_count
292 self._bytes_since_update += byte_count
293 now = time.time()
294+ if self._total_byte_count < 2000:
295+ # a little resistance at first, so it doesn't stay stuck at 0
296+ # while connecting...
297+ return
298 if self._transport_update_time is None:
299 self._transport_update_time = now
300 elif now >= (self._transport_update_time + 0.5):