Code review comment for lp:~jtv/launchpad/bug-507678

= Bug 507678 =

Continuing work on making the buildfarm generate translation templates from source branches. Here I'm jumping through the hoops to let the IBuildFarmJobBehavior implementation that we have for this kind of build-farm job retrieve a tarball produced by the slave.

The default BuildFarmJobBehaviorBase code for this assumed that there's an IBuildBase object (basically a generalization of soyuz's Build) attached to a build-farm job. It didn't seem worth it to implement IBuildBase just for this (I tried), I now override the only BuildFarmJobBehaviorBase method that requires it. I had to extend the infrastructure a bit along the way.

An important thing to note is that these pieces are still coming together; they're not actually a functional whole yet. But with the ongoing work on build-farm generalization, it's important to get and keep them integrated before bit-rot sets in.

Going through the diff step by step:

lib/lp/buildmaster/model/buildfarmjobbehavior.py

I extracted a method for reading and checking build status from a slave status dict. That makes it easy for my behavior class to reuse it.

lib/lp/buildmaster/tests/test_buildfarmjobbehavior.py

Tests the method I abstracted.

lib/lp/testing/factory.py

New factory method for the type of build-farm job that generates translation templates.

lib/lp/testing/fakemethod.py

Provides a generic stub for all sorts of functions and methods. I got tired of writing one-liners, injecting ad-hoc status variables into objects, and building one-off mock classes. Need to override a method in an object to return a dummy value or fail in a particular way? Just assign it to a FakeMethod.

Pylint had a truly silly complaint about this file, so I silenced it. There's a comment to the effect.

lib/lp/testing/tests/test_fakemethod.py

Unit-tests FakeMethod.

lib/lp/translations/model/translationtemplatesbuildbehavior.py

Grew some methods for retrieving a tarball of results from the build-farm slave. As yet we do not have the code in place that will generate the real templates.

lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py

Tests the extensions to the behavior class. The starting point for this was an existing unit test that I ripped out of the next file in the diff.

Interaction with slaves makes testing this stuff very hard. About the best I can do is mock up all the infrastructure, unit-test my part, and hope the other classes do their job. It's not perfect, but it's a lot more automated testing than a lot of the other buildfarm work gets.

For this test I had to strike a balance black-box and white-box approaches: obviously the test needs a lot of inside knowledge about the classes it's dealing with, but I didn't make it check the fidgety details of what values are being passed around where. Those could change easily, as work on the build farm is still ongoing. Moreover, I have no real requirements for them other than "they should work."

lib/lp/translations/tests/test_translationtemplatesbuildjob.py

Lost a test case to the file above. What there was got simplified a bit, and setUp got replaced with explicit creation of objects.

To test these changes:
{{{
./bin/test -vv -t buildfarmjobbehavior -t translationtemplatesbuild -t fakemethod
}}}

There's not much to Q/A, since the pieces are still coming together.

One piece of lint shows up, but it's unrelated to my changes:
{{{
lib/lp/testing/factory.py
    1587: [F0401, LaunchpadObjectFactory.makeRecipe] Unable to import 'bzrlib.plugins.builder.recipe' (No module named builder)
}}}

Not sure what I could reasonably do about this. The structure of that code suggests it's expected, and disabling a warning for this whole file would be insane.

Jeroen