Code review comment for lp:~benji/launchpad/bug-953913

Hi guys, sorry for taking a look at this so late (I wasn't working the Thu/Fri).

I find it interesting that this has failed intermittently. Let me digest what happens in a test (the fact that this fix is here helps with that a lot :).

`potemplate` is the template we are renaming with one `potmsgset`.
`old_shared` is sharing with `potemplate` that same potmsgset.
`new_shared` is a different template with a different `target_potmsgset` but identical English text for it.

`potemplate` is renamed to start sharing with `new_shared`.

Two things happen:
 1. splitting: `potmsgset` is split into two similar ones: one in `potemplate`, another in `old_shared` (this should have nothing to do with development_focus)
 2. merging: whatever came out of 1 and ended up in `old_shared` (could be `potmsgset` itself or a different potmsgset object), and `target_potmsgset` into a single potmsgset, with the development_focus potmsgset being preferred for keeping.

So, if there's any instability (for this particular test), it's probably in how splitting is done (i.e. sometimes we get the translation_focus `potmsgset` in old_shared, sometimes a new POTMsgSet), and it'd probably make sense to make that use sharingKey as well, i.e. prefer to keep the translation_focus potmsgset object where it is. Then, in 1, we'd always have `potmsgset` remain in `potemplate`, and `old_shared` will end up with eg. `split_potmsgset`. But then, both of `split_potmsgset` and `target_potmsgset` and which one is kept on merging would depend on their IDs, which in testing we can probably rely on to grow sequentially, but shouldn't.

Basically, this code produces a few equivalent, but non-identical "solutions" to the splitting/merging problem. It makes it harder to unit-test, which is likely why I introduced the problem :)

So, IMHO, for this particular test, it would be better to remove the dependency on the development_focus altogether. I suspect having

=== modified file 'lib/lp/translations/tests/test_translationpackagingjob.py'
--- lib/lp/translations/tests/test_translationpackagingjob.py 2012-03-23 21:06:28 +0000
+++ lib/lp/translations/tests/test_translationpackagingjob.py 2012-03-26 07:22:36 +0000
@@ -340,7 +340,9 @@
         # New POTMsgSet is now different from the old one (it's been split),
         # but matches the target potmsgset (it's been merged into it).
         new_potmsgset = potemplate.getPOTMsgSets()[0]
- self.assertNotEqual(potmsgset, new_potmsgset)
+ old_potmsgset = old_shared.getPOTMsgSets()[0]
+ target_potmsgset = new_shared.getPOTMsgSets()[0]
+ self.assertNotEqual(old_potmsgset, new_potmsgset)
         self.assertEqual(target_potmsgset, new_potmsgset)

         # Translations have been merged as well.

would be a sufficient (and more correct) fix instead of the one here (though, adding a comment why is this needed to refetch all the potmsgsets would be rather helpful :).