Code review comment for lp:~mvo/python-apt/mvo

Revision history for this message
Michael Vogt (mvo) wrote :

On Mon, Oct 15, 2012 at 05:08:22PM -0000, Jason Conti wrote:
> Review: Approve
>
> Looks good to me overall. My only concern is that test_cache_delete_leasks_fds() may fail in certain circumstances, and it happened to fail in my test rebuild. Python does eventually get around to deleting the records instance.

Thanks a lot! Oh, interessting. I had hoped that the gc.collect()
would ensure that its deleted and thereforce closed. Does the test
fails reliable for you? If so, any hints what I can do trying to
reproduce the issue?

> I like the other additional tests, and actually had a very similar test to test_cache_close_download_fails() but it didn't occur to me to include it for some reason. It also appears I completely missed the case about leaking fds on cache reopen, thanks for fixing it.

Great, thanks for the review!

Cheers,
 Michael

« Back to merge proposal