Opened 6 years ago

Last modified 6 years ago

#5680 defect new

test_timelyProcessExited is fragile and may not catch actual problems

Reported by: Itamar Turner-Trauring Owned by: Itamar Turner-Trauring
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/process-test-5680
branch-diff, diff-cov, branch-cov, buildbot
Author: itamar


JP pointed out that test_timelyProcessExited in twisted.internet.test_process may fail without us noticing:

The problem is that the timeout support in runReactor is based on the IReactorTime implementation of the reactor it is running. It's quite likely that if the reactor is broken, instead of failing:

  1. the child will exit
  2. the reactor will not process the signal right away
  3. the timeout will elapse
  4. the reactor will wake up
  5. the signal will be processed, calling processEnded
  6. reactor.stop() will be called
  7. runReactor will notice the reactor was stopped and not raise a timeout exception
  8. the test will pass

Change History (3)

comment:1 Changed 6 years ago by itamarst

Author: itamarst
Branch: branches/process-test-5680

(In [34411]) Branching to 'process-test-5680'

comment:2 Changed 6 years ago by Itamar Turner-Trauring

Author: itamarstitamar
Keywords: review added
Owner: Itamar Turner-Trauring deleted

Ready for review; also covers #5679. I scheduled but haven't yet checked a buildbot run:

comment:3 Changed 6 years ago by therve

Keywords: review removed
Owner: set to Itamar Turner-Trauring

Looks good. I would add a parameter to assertTrue with the value of exist - start so that we can debug it in case of problems. Please merge after that.

Note: See TracTickets for help on using tickets.