Opened 11 years ago

Closed 11 years ago

#1883 task closed fixed (fixed)

Make selected tests clean up after themselves

Reported by: Jonathan Lange Owned by:
Priority: highest Milestone: Core-2.5
Component: core Keywords: tests
Cc: Jean-Paul Calderone, Stephen Thorne Branch:
Author:

Description

Trial's clean up code spins the reactor a couple of times before cleaning up pending calls. This won't work if the tests are run in a reactor. Removing the iterate calls make a whole bunch of tests spit out cleanup errors.

Change History (18)

comment:1 Changed 11 years ago by Jonathan Lange

Roughly 120+ Unclean Reactor warnings when the lines are removed.

comment:2 Changed 11 years ago by Jonathan Lange

Cc: Jean-Paul Calderone Stephen Thorne added

OK. So maybe this can't be done by cleaning up tests, maybe the problem is best fixed in the implementation of do_cleanPending.

It can't call reactor.iterate(0), because it needs to be run in a callback.

comment:3 Changed 11 years ago by Jonathan Lange

Priority: normalhigh

comment:4 Changed 11 years ago by Jonathan Lange

Status: newassigned

comment:5 Changed 11 years ago by jknight

I would suggest making it do reactor.callFromThread(finish) instead. That is pretty much an equivalent of reactor.iterate(0).

comment:6 Changed 11 years ago by Jean-Paul Calderone

reactor.callLater(0, ...) twice should exactly duplicate the current behavior. I think using callLater is a little less obscure than using callFromThread. callFromThread also isn't available on platforms without threading support.

comment:7 Changed 11 years ago by jknight

I agree that it should, but it doesn't, due to the callLater ordering bug (#1396) that I said I would fix but have not merged. =/

comment:8 Changed 11 years ago by Jean-Paul Calderone

Does any code actually depend on that difference? It's pretty subtle, and the point of the iterate() calls are just to get rid of anything that's almost done.

Ultimately, things which are still active at this point in a test should probably just be considered failures, so we just need a solution that's good enough to get us far enough to start fixing those things.

comment:9 Changed 11 years ago by Jonathan Lange

Jp, what do you mean by "good enough to get us far enough to start fixing those things". After all, removing the iterate calls makes it fairly clear which tests are failing.

comment:10 Changed 11 years ago by jknight

Difference in ordering? Yes, simplified example:

# Test case code:
  reactor.callLater(0, lambda: None)
# Trial code:
  reactor.callLater(0, finishCleanUp)

Because of the bug, finishCleanUp sometimes will get called before frobnitz (depending on system clock resolution and timing). Therefore, if the goal is to just let things almost done finish, that is not a suitably reliable way to do so.

reactor.callLater(0.000001, finishCleanUp), would work, though.

comment:11 Changed 11 years ago by Jonathan Lange

Milestone: Twisted-2.5

comment:12 Changed 11 years ago by Jonathan Lange

Keywords: review added
Owner: changed from Jonathan Lange to Stephen Thorne
Priority: highhighest
Status: assignednew

Ugh. This branch has been around for ages and is not going anywhere anytime soon.

I'm putting it up for review -- it fixes a couple of dodgy tests, those fixes should get merged.

comment:13 Changed 11 years ago by Jonathan Lange

Summary: Remove reactor.iterate calls from do_cleanPendingMake selected tests clean up after themselves

comment:14 Changed 11 years ago by Cory Dodt

Review:

  • commented out the reactor.iterate(0) calls, and unclean reactor appears. Apply the patch for this bug, and unclean reactor goes away. Ergo, this patch fixes what it says it fixes.
  • Did not introduce any test regressions.

review = ok.

comment:15 Changed 11 years ago by Cory Dodt

Keywords: review removed

comment:16 Changed 11 years ago by Jonathan Lange

Owner: changed from Stephen Thorne to Jonathan Lange
Status: newassigned

comment:17 Changed 11 years ago by Jonathan Lange

Resolution: fixed
Status: assignedclosed

(In [18220]) Make tests clean up after themselves

  • Fixes #1883
  • Author: jml
  • Reviewer: MFen

Although not apparent to the naked eye, these tests previous did not correctly clean up after themselves. Instead, they relied on a couple of spins of the reactor to do their cleaning for them. This has been corrected.

comment:18 Changed 7 years ago by <automation>

Owner: Jonathan Lange deleted
Note: See TracTickets for help on using tickets.