Opened 16 years ago

Closed 15 years ago

#1507 defect closed fixed (fixed)

trial doesn't gc.collect between test methods

Reported by: jknight Owned by:
Priority: highest Milestone: Core-2.5
Component: trial Keywords:
Cc: Glyph, jknight, itamarst, radix, Jean-Paul Calderone, spiv, warner, Jonathan Lange Branch:
Author:

Description

In particular:

Exception exceptions.OSError: (2, 'No such file or directory', '/tmp/tmpHNWi8_') in <bound method _TemporaryFileWrapper.__del__ of <closed file '<fdopen>', mode 'w+b' at 0x14dea60>> ignored

is appearing at random points after the test that causes it.

An extra-bonus-nice thing would be for trial to magically capture said error and fail the test because of it.

Change History (35)

comment:1 Changed 16 years ago by Jonathan Lange

Status: newassigned

There's a fix to call gc.collect in source:branches/gccollect-1507 Pending review.

comment:2 Changed 16 years ago by Jonathan Lange

Keywords: review added

comment:3 Changed 16 years ago by Jonathan Lange

Owner: changed from Jonathan Lange to Jean-Paul Calderone
Status: assignednew

This branch will fix some of the problems reported in #1213

comment:4 Changed 16 years ago by Jean-Paul Calderone

Owner: changed from Jean-Paul Calderone to Jonathan Lange

twisted/trial/test/test_tests.py imports gc twice. The second import looks extremely broken and should probably go away. Also, stdlib imports should be together, before Twisted imports.

test_collectCalled's treatment of _collectCalled is odd. Why isn't this an instance attribute?

twisted.trial.test.weird does not seem to have been added to the repository.

The protected import of gc in twisted/trial/unittest.py seems pointless, unless all the other gc imports are going to be guarded too.

Pyflakes has this to say about unittest.py:

twisted/trial/unittest.py:7: 'errno' imported but unused twisted/trial/unittest.py:7: 'time' imported but unused twisted/trial/unittest.py:10: 'reflect' imported but unused twisted/trial/unittest.py:12: 'deferredError' imported but unused twisted/trial/unittest.py:12: 'deferredResult' imported but unused twisted/trial/unittest.py:443: redefinition of unused 'gc' from line 441 twisted/trial/unittest.py:559: undefined name 'WaitIsNotReentrantError' twisted/trial/unittest.py:559: undefined name 'REENTRANT_WAIT_ERROR_MSG'

And this to say about test_tests.py:

twisted/trial/test/test_tests.py:90: undefined name 'SkipTest' twisted/trial/test/test_tests.py:140: undefined name 'SkipTest' twisted/trial/test/test_tests.py:467: redefinition of unused 'gc' from line 6 twisted/trial/test/test_tests.py:469: undefined name 'TestGarbageCollection'

comment:5 Changed 16 years ago by Jean-Paul Calderone

Priority: normalhighest

comment:6 Changed 16 years ago by Jonathan Lange

(In [16160]) Apply feedback from review. See #1507

comment:7 Changed 16 years ago by Jonathan Lange

Owner: changed from Jonathan Lange to Jean-Paul Calderone

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

Cc: Glyph jknight itamarst radix Jean-Paul Calderone spiv added

The only remaining problem I can see is that the tests are now dog slow. :/

comment:9 Changed 16 years ago by itamarst

We could have an optional --use-gc flag? Or, have a --aggressive or something flag that does gc and whatever other extra testing we think is useful but is too slow? See #792 as well.

comment:10 Changed 16 years ago by Jean-Paul Calderone

Cc: warner added

This branch basically fixes a regression introduced by r12088. That is also the revision which made trial 10x faster. What an interesting coincidence.

By the way, when I said "dog slow" I meant "Twisted's test suite now takes 6461 seconds on a 2GHz AMD 64". Although some large part of that may just be due to newpb tests failing and timing out.

Not doing the collect introduces spurious failures that are difficult to debug. I don't think it's really an option to continue running tests incorrectly like this. Having an option to turn this behavior _off_ _might_ be an option, but that seems pretty sucktastic to me. Why not just add a --pass-everything flag while we're at it?

I think we should consider freezing development and making the test suite suck less.

comment:11 Changed 16 years ago by Jonathan Lange

  • +1 on freezing development and making the test suite suck less.
  • --pass-everything exists as --dry-run
  • Are there options for dealing with unhandled Deferreds other than gc.collect()? Some sort of Deferred-creation hook maybe?

comment:12 Changed 16 years ago by Jean-Paul Calderone

Cc: Jonathan Lange added
Keywords: review removed
Owner: changed from Jean-Paul Calderone to Jonathan Lange

Okay, I'd really like to get this merged.

Do the newpb tests still fail deterministically in this branch?

If so, warner should take a look. He has mentioned that he has been unable to get them to fail deterministically. If they don't look easily fixable, I guess we should go ahead with the plan to disable the newpb tests, although I realize no one is particularly happy with that plan.

comment:13 Changed 16 years ago by Jonathan Lange

What happened to "the tests are dog slow"?

comment:14 Changed 16 years ago by Jonathan Lange

Owner: changed from Jonathan Lange to warner

comment:15 Changed 16 years ago by Jonathan Lange

Owner: changed from warner to Jonathan Lange

Warner has fixed the tests. Are we going to merge?

comment:16 Changed 16 years ago by radix

FYI

This change increases overhead by 100 (according to a test I did where I wrote a test with 1000 test methods that only did 'pass'; with gc.collect()s commented out, it took about 1.6 seconds. With them enabled, it took over 100 seconds. Actual twisted test runs are "only" about ten times slower.

I'm sorry. :-(

comment:17 Changed 16 years ago by Jonathan Lange

OK, so we aren't going to merge.

What shall we do instead?

comment:18 Changed 16 years ago by Jonathan Lange

Resolution: wontfix
Status: newclosed

Applying this branch makes the tests correct but around ten times slower. This isn't acceptable, so marking as wontfix.

If you feel this is premature, reopen the ticket but please do not assign it to me. I can do nothing about it.

comment:19 Changed 16 years ago by jknight

Resolution: wontfix
Status: closedreopened

There should at least be a flag to enable it. Then when you do have the problem (which you can tell if you're observant, because it shows up in the logs), there's at least the possibility of tracking it down without tearing your hair out.

I think this would be a good compromise within the constraints.

comment:20 Changed 16 years ago by Jonathan Lange

Jp, you've objected to having an option before. Have you changed your mind?

comment:21 Changed 16 years ago by Jonathan Lange

Owner: changed from Jonathan Lange to Jean-Paul Calderone
Status: reopenednew

comment:22 Changed 16 years ago by Jean-Paul Calderone

Owner: changed from Jean-Paul Calderone to Jonathan Lange

I still don't like having this as an option, but the current situation is even worse, so I guess it should be added as an option.

comment:23 Changed 16 years ago by Jonathan Lange

Status: newassigned

comment:24 Changed 16 years ago by Jonathan Lange

Keywords: review added
Owner: changed from Jonathan Lange to Jean-Paul Calderone
Status: assignednew

My thoughts exactly.

Now a flag (--use-gc) in source:branches/gccollect-1507-2

comment:25 Changed 16 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Jonathan Lange

Is a private class attribute really the best way to implement this switch? Can't we pass some arguments through somewhere instead?

The PB change is pretty nasty, but arguably that is a bug in pb or the pb tests, and they fail in trunk already anyway so I don't think it needs to block this branch.

GCMixin should have some docstrings and such.

Last thing, is --use-gc the best name for this option? It seems to imply that no gc will happen if it is omitted.

comment:26 Changed 16 years ago by Jonathan Lange

Keywords: review added
Owner: changed from Jonathan Lange to Jean-Paul Calderone
  • Now it's a public instance attribute, set by the TestLoader
  • Docstrings for !GCMixin
  • Option changed to --force-gc

comment:27 Changed 16 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Jonathan Lange

(note, code is in gccollect-1507-3 now)

Looking good. Please add docstrings for test_collectNotDefault, test_isReported, test_doesntBleed, test_forcedGc, test_loadWithForcedGarbageCollection, test_loadWithForcedGarbedCollectionClass, and a class docstring for TestLoader and TestCase which at least documents the forceGarbageCollection attribute.

Thank you for updating the man page :)

Feel free to merge without further review once the above mentioned docstrings are written.

comment:28 Changed 16 years ago by Jean-Paul Calderone

Okay I lied again: twisted/trial/runner.py:334 unconditionally sets forceGarbageCollection to True on the test cases it creates. Clearly this is not the correct behavior.

comment:29 Changed 15 years ago by Jonathan Lange

Status: newassigned

comment:30 Changed 15 years ago by Jonathan Lange

Milestone: Twisted-2.5

comment:31 Changed 15 years ago by Jonathan Lange

This must go in to 2.5.

comment:32 Changed 15 years ago by Jonathan Lange

Keywords: review added
Owner: changed from Jonathan Lange to Jean-Paul Calderone
Status: assignednew

Added doctrings, and also another couple of tests (which have docstrings).

It seems that I documented TestLoader, TestCase and corrected the 'unconditionally setting forceGarbageCollection to true' problem some time ago.

Back to you.

comment:33 Changed 15 years ago by Stephen Thorne

Keywords: review removed
Owner: changed from Jean-Paul Calderone to Jonathan Lange

twisted.trial.runner has an odd style:

-        tests = self.sort([ klass(self.methodPrefix+name) for name in names ])
+        tests = self.sort([ self._makeCase(klass, self.methodPrefix+name)
+                            for name in names ])

please remove the extra spaces.

Pyflakes errors are here:

twisted/pb/test/common.py:11: 'CONNECTION_LOST' imported but unused
twisted/pb/test/common.py:36: undefined name 'log'
twisted/trial/test/weird.py:2: 'failure' imported but unused
twisted/trial/test/weird.py:4: 'gc' imported but unused
twisted/trial/runner.py:485: 'readline' imported but unused

please correct or justify quietly to yourself these pyflakes errors.

Tests are there, tests pass clean. I love tests. Because you've got tests I'm going to say, "please merge this after you fix the above niggles". :)

comment:34 Changed 15 years ago by Jonathan Lange

Resolution: fixed
Status: newclosed

(In [18207]) Add --force-gc option to trial, to force garbage collection between tests

  • Author: jml
  • Reviewer: Jerub
  • Fixes #1507

Ideally, Trial should always force garbage collection between tests. Unfortunately, this makes the test suite take ten times as long to complete. As a compromise, we make the correct behaviour an option.

comment:35 Changed 11 years ago by <automation>

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