Code review comment for lp:~gz/bzr/cleanup_testcases_by_collection_613247

Hmm, I appreciate the effort to reduce the memory footprint, but unfortunately this seems far from landable :-/

BZR_PLUGIN_PATH=-site ./bzr selftest --parallel=fork
bzr selftest: /home/vila/src/bzr/reviews/cleanup_testcases_by_collection_613247/bzr
   /home/vila/src/bzr/reviews/cleanup_testcases_by_collection_613247/bzrlib
   bzr-2.2b3 python-2.6.5 Linux-2.6.32-24-generic-x86_64-with-Ubuntu-10.04-lucid

----------------------------------------------------------------------
Ran 8 tests in 0.489s

OK
2 tests skipped

Huh ? Where are my tests gone ? (Strong suspicion around your modification of __iter__)

Ran 24856 tests in 1050.614s

FAILED (failures=2, errors=2, known_failure_count=48)

Including failures for bb.test_log so your fix is wrong there.

On the overall, with a superficial reading, the way you avoid ref cycles seem hard to understand
so very likely to be re-introduced. How about breaking the ref-cycles with addCleanup calls instead
(by deleting the offending attributes) ?

Now, a way to track the uncollectable test case sounds like a good way to progress but given the amount of work, could you instead devise a patch where the check depends on some option ?
Once we reach 0 we can toggle the default value for this option.

For the run above, I also get ~4000 Uncollected test cases, far too much to land this patch :-/