Merge lp:~benji/testtools/modernize-tsfr into lp:~testtools-committers/testtools/trunk

Proposed by Benji York
Status: Rejected
Rejected by: Jonathan Lange
Proposed branch: lp:~benji/testtools/modernize-tsfr
Merge into: lp:~testtools-committers/testtools/trunk
Prerequisite: lp:~jml/testtools/forward-current-tags
Diff against target: 266 lines (+51/-138)
2 files modified
testtools/testresult/real.py (+24/-62)
testtools/tests/test_testresult.py (+27/-76)
To merge this branch: bzr merge lp:~benji/testtools/modernize-tsfr
Reviewer Review Type Date Requested Status
Jonathan Lange Disapprove
Review via email: mp+101749@code.launchpad.net

Commit message

Modernise ThreadSafeForwardingResult by simplifying it and adding some tests.

Description of the change

The ThreadSafeForwardingResult attempts to manage tag state itself
because the wrapped test result did not previously manage tags
correctly. The recent ~jml/testtools/forward-current-tags branch fixes
those issues so TSFR needed to be updated. This branch does that by
simplifying TSFR, removing some now unneeded tests, and adding some new
tests.

To post a comment you must log in.
Revision history for this message
Jonathan Lange (jml) wrote :

Hey Benji,

First up, thanks so much for the contribution, and for valiantly persisting in cleaning up this somewhat unclear code. Sadly, I'm afraid it's back to the drawing with this one. I'm afraid it's our fault for not clearly communicating the intent of TSFR.

TSFR's main purpose is to send all of the events for a single test in an atomic unit, preventing the destination result from receiving input like this:

  startTest(foo)
  startTest(bar)
  addSuccess(foo)
  addSuccess(bar)
  stopTest(bar)
  stopTest(foo)

And instead guaranteeing input like this:

  startTest(bar)
  addSuccess(bar)
  stopTest(bar)
  startTest(foo)
  addSuccess(foo)
  stopTest(foo)

If I understand this patch correctly, it wraps each individual event in a semaphore, which isn't enough to guarantee this per-test atomicity.

I'll try to sketch out, or possibly even implement, a solution here.

jml

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

In fact TSFR *must* manage tag state itself; it has to supply either
the delta (by inspection) to what its state is at each synchronisation
point, or it has to supply a back-out-its-own-state command at the end
of each synchronisation point.

I'd be inclined to do the latter.

Do you have a SSCCE of the problem?

-Rob

Unmerged revisions

270. By Benji York

tweak comment

269. By Benji York

add a test that demonstrates ThreadsafeForwardingResult's use of semaphores

268. By Benji York

simplify tests some more and fix recently-introduced bug in wasSuccessful

267. By Benji York

wasSuccessful might do anything, so wrap it in the semaphore too

266. By Benji York

regularize semaphore use

265. By Benji York

cleanup some unneeded concepts that I recently introduced

264. By Benji York

get ThreadsafeForwardingResult tests passing

263. By Benji York

checkpoint - all tag tests pass

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'testtools/testresult/real.py'
--- testtools/testresult/real.py 2012-04-12 15:07:32 +0000
+++ testtools/testresult/real.py 2012-04-12 15:07:32 +0000
@@ -376,110 +376,72 @@
376 TestResult.__init__(self)376 TestResult.__init__(self)
377 self.result = ExtendedToOriginalDecorator(target)377 self.result = ExtendedToOriginalDecorator(target)
378 self.semaphore = semaphore378 self.semaphore = semaphore
379 self._test_start = None
380 self._global_tags = set(), set()379 self._global_tags = set(), set()
381 self._test_tags = set(), set()380 self._test_tags = set(), set()
382381
383 def __repr__(self):382 def __repr__(self):
384 return '<%s %r>' % (self.__class__.__name__, self.result)383 return '<%s %r>' % (self.__class__.__name__, self.result)
385384
386 def _not_empty_tags(self, tags):385 @property
387 return bool(tags[0] and tags[1])386 def current_tags(self):
387 self.semaphore.acquire()
388 try:
389 return self.result.current_tags
390 finally:
391 self.semaphore.release()
388392
389 def _add_result_with_semaphore(self, method, test, *args, **kwargs):393 def _call_with_semaphore(self, method, *args, **kwargs):
390 now = self._now()
391 self.semaphore.acquire()394 self.semaphore.acquire()
392 try:395 try:
393 self.result.time(self._test_start)396 return method(*args, **kwargs)
394 if self._not_empty_tags(self._global_tags):
395 self.result.tags(*self._global_tags)
396 try:
397 self.result.startTest(test)
398 self.result.time(now)
399 if self._not_empty_tags(self._test_tags):
400 self.result.tags(*self._test_tags)
401 try:
402 method(test, *args, **kwargs)
403 finally:
404 self.result.stopTest(test)
405 finally:
406 if self._not_empty_tags(self._global_tags):
407 self.result.tags(*reversed(self._global_tags))
408 finally:397 finally:
409 self.semaphore.release()398 self.semaphore.release()
410 self._test_start = None
411399
412 def addError(self, test, err=None, details=None):400 def addError(self, test, err=None, details=None):
413 self._add_result_with_semaphore(self.result.addError,401 self._call_with_semaphore(self.result.addError,
414 test, err, details=details)402 test, err, details=details)
415403
416 def addExpectedFailure(self, test, err=None, details=None):404 def addExpectedFailure(self, test, err=None, details=None):
417 self._add_result_with_semaphore(self.result.addExpectedFailure,405 self._call_with_semaphore(self.result.addExpectedFailure,
418 test, err, details=details)406 test, err, details=details)
419407
420 def addFailure(self, test, err=None, details=None):408 def addFailure(self, test, err=None, details=None):
421 self._add_result_with_semaphore(self.result.addFailure,409 self._call_with_semaphore(self.result.addFailure,
422 test, err, details=details)410 test, err, details=details)
423411
424 def addSkip(self, test, reason=None, details=None):412 def addSkip(self, test, reason=None, details=None):
425 self._add_result_with_semaphore(self.result.addSkip,413 self._call_with_semaphore(self.result.addSkip,
426 test, reason, details=details)414 test, reason, details=details)
427415
428 def addSuccess(self, test, details=None):416 def addSuccess(self, test, details=None):
429 self._add_result_with_semaphore(self.result.addSuccess,417 self._call_with_semaphore(self.result.addSuccess,
430 test, details=details)418 test, details=details)
431419
432 def addUnexpectedSuccess(self, test, details=None):420 def addUnexpectedSuccess(self, test, details=None):
433 self._add_result_with_semaphore(self.result.addUnexpectedSuccess,421 self._call_with_semaphore(self.result.addUnexpectedSuccess,
434 test, details=details)422 test, details=details)
435423
436 def startTestRun(self):424 def startTestRun(self):
437 super(ThreadsafeForwardingResult, self).startTestRun()425 self._call_with_semaphore(self.result.startTestRun)
438 self.semaphore.acquire()
439 try:
440 self.result.startTestRun()
441 finally:
442 self.semaphore.release()
443426
444 def stopTestRun(self):427 def stopTestRun(self):
445 self.semaphore.acquire()428 self._call_with_semaphore(self.result.stopTestRun)
446 try:
447 self.result.stopTestRun()
448 finally:
449 self.semaphore.release()
450429
451 def done(self):430 def done(self):
452 self.semaphore.acquire()431 self._call_with_semaphore(self.result.done)
453 try:
454 self.result.done()
455 finally:
456 self.semaphore.release()
457432
458 def startTest(self, test):433 def startTest(self, test):
459 self._test_start = self._now()434 self._call_with_semaphore(self.result.startTest, test)
460 super(ThreadsafeForwardingResult, self).startTest(test)435
436 def stopTest(self, test):
437 self._call_with_semaphore(self.result.stopTest, test)
461438
462 def wasSuccessful(self):439 def wasSuccessful(self):
463 return self.result.wasSuccessful()440 return self._call_with_semaphore(self.result.wasSuccessful)
464441
465 def tags(self, new_tags, gone_tags):442 def tags(self, new_tags, gone_tags):
466 """See `TestResult`."""443 """See `TestResult`."""
467 super(ThreadsafeForwardingResult, self).tags(new_tags, gone_tags)444 self._call_with_semaphore(self.result.tags, new_tags, gone_tags)
468 if self._test_start is not None:
469 self._test_tags = self._merge_tags(
470 self._test_tags, new_tags, gone_tags)
471 else:
472 self._global_tags = self._merge_tags(
473 self._global_tags, new_tags, gone_tags)
474
475 def _merge_tags(self, existing, new_tags, gone_tags):
476 result_new = set(existing[0])
477 result_gone = set(existing[1])
478 result_new.update(new_tags)
479 result_new.difference_update(gone_tags)
480 result_gone.update(gone_tags)
481 result_gone.difference_update(new_tags)
482 return result_new, result_gone
483445
484446
485class ExtendedToOriginalDecorator(object):447class ExtendedToOriginalDecorator(object):
486448
=== modified file 'testtools/tests/test_testresult.py'
--- testtools/tests/test_testresult.py 2012-04-12 15:07:32 +0000
+++ testtools/tests/test_testresult.py 2012-04-12 15:07:32 +0000
@@ -103,6 +103,18 @@
103 return sys.exc_info()103 return sys.exc_info()
104104
105105
106class SemaphoreDouble(object):
107
108 def __init__(self, log):
109 self.log = log.append
110
111 def acquire(self):
112 self.log('acquired')
113
114 def release(self):
115 self.log('released')
116
117
106class Python26Contract(object):118class Python26Contract(object):
107119
108 def test_fresh_result_is_successful(self):120 def test_fresh_result_is_successful(self):
@@ -766,13 +778,6 @@
766 self.result1 = ThreadsafeForwardingResult(self.target,778 self.result1 = ThreadsafeForwardingResult(self.target,
767 self.result_semaphore)779 self.result_semaphore)
768780
769 def test_nonforwarding_methods(self):
770 # startTest and stopTest are not forwarded because they need to be
771 # batched.
772 self.result1.startTest(self)
773 self.result1.stopTest(self)
774 self.assertEqual([], self.target._events)
775
776 def test_startTestRun(self):781 def test_startTestRun(self):
777 self.result1.startTestRun()782 self.result1.startTestRun()
778 self.result2 = ThreadsafeForwardingResult(self.target,783 self.result2 = ThreadsafeForwardingResult(self.target,
@@ -787,75 +792,21 @@
787 self.result2.stopTestRun()792 self.result2.stopTestRun()
788 self.assertEqual(["stopTestRun", "stopTestRun"], self.target._events)793 self.assertEqual(["stopTestRun", "stopTestRun"], self.target._events)
789794
790 def test_forwarding_methods(self):795 def test_semaphore_aquired(self):
791 # error, failure, skip and success are forwarded in batches.796 # The methods (and properties) of TSFR (ThreadsafeForwardingResult)
792 exc_info1 = make_exception_info(RuntimeError, 'error')797 # are protected by a semaphore so only one thread may access the
793 starttime1 = datetime.datetime.utcfromtimestamp(1.489)798 # underlying test result at any one time. This can be demonstrated by
794 endtime1 = datetime.datetime.utcfromtimestamp(51.476)799 # inserting our own logging semaphore double and watching its
795 self.result1.time(starttime1)800 # behavior.
796 self.result1.startTest(self)801 log = []
797 self.result1.time(endtime1)802 result = LoggingResult(log)
798 self.result1.addError(self, exc_info1)803 semaphore = SemaphoreDouble(log)
799 exc_info2 = make_exception_info(AssertionError, 'failure')804 tsfr = ThreadsafeForwardingResult(result, semaphore)
800 starttime2 = datetime.datetime.utcfromtimestamp(2.489)805 # Now if we make calls on the TSFR the underlying result will be
801 endtime2 = datetime.datetime.utcfromtimestamp(3.476)806 # called too and the semaphore will be acquired and released around
802 self.result1.time(starttime2)807 # the calls.
803 self.result1.startTest(self)808 tsfr.startTestRun()
804 self.result1.time(endtime2)809 self.assertEqual(['acquired', 'startTestRun', 'released'], log)
805 self.result1.addFailure(self, exc_info2)
806 reason = _u("Skipped for some reason")
807 starttime3 = datetime.datetime.utcfromtimestamp(4.489)
808 endtime3 = datetime.datetime.utcfromtimestamp(5.476)
809 self.result1.time(starttime3)
810 self.result1.startTest(self)
811 self.result1.time(endtime3)
812 self.result1.addSkip(self, reason)
813 starttime4 = datetime.datetime.utcfromtimestamp(6.489)
814 endtime4 = datetime.datetime.utcfromtimestamp(7.476)
815 self.result1.time(starttime4)
816 self.result1.startTest(self)
817 self.result1.time(endtime4)
818 self.result1.addSuccess(self)
819 self.assertEqual([
820 ('time', starttime1),
821 ('startTest', self),
822 ('time', endtime1),
823 ('addError', self, exc_info1),
824 ('stopTest', self),
825 ('time', starttime2),
826 ('startTest', self),
827 ('time', endtime2),
828 ('addFailure', self, exc_info2),
829 ('stopTest', self),
830 ('time', starttime3),
831 ('startTest', self),
832 ('time', endtime3),
833 ('addSkip', self, reason),
834 ('stopTest', self),
835 ('time', starttime4),
836 ('startTest', self),
837 ('time', endtime4),
838 ('addSuccess', self),
839 ('stopTest', self),
840 ], self.target._events)
841
842 def test_tags_helper(self):
843 expected = set(['present']), set(['missing', 'going'])
844 input = set(['present']), set(['missing'])
845 self.assertEqual(
846 expected, self.result1._merge_tags(input, set(), set(['going'])))
847 expected = set(['present']), set(['missing', 'going'])
848 input = set(['present', 'going']), set(['missing'])
849 self.assertEqual(
850 expected, self.result1._merge_tags(input, set(), set(['going'])))
851 expected = set(['coming', 'present']), set(['missing'])
852 input = set(['present']), set(['missing'])
853 self.assertEqual(
854 expected, self.result1._merge_tags(input, set(['coming']), set()))
855 expected = set(['coming', 'present']), set(['missing'])
856 input = set(['present']), set(['coming', 'missing'])
857 self.assertEqual(
858 expected, self.result1._merge_tags(input, set(['coming']), set()))
859810
860811
861class TestExtendedToOriginalResultDecoratorBase(TestCase):812class TestExtendedToOriginalResultDecoratorBase(TestCase):

Subscribers

People subscribed via source and target branches