Code review comment for lp:~pwlars/lava-test/list

On Thu, 08 Jul 2010 21:50:58 -0000, Paul Larson <email address hidden> wrote:
> Paul Larson has proposed merging lp:~pwlars/abrek/list into lp:abrek.
>
> Requested reviews:
> Linaro Infrastructure (arm-infrastructure)
>
>
> These two commands are pretty straightforward - one to list the tests that have been defined, another to list the tests that have been installed. Not sure how to go about testing them since it prints it directly, unless I separate out a function to generate the list. Considering how simple they are right now, and the limited usefulness of such a function in other places, I think that might be overkill. But if you disagree, or have a better idea for how to unit test this, I'm all ears.

So, while I agree there isn't a lot of value to testing these things,
the danger is that you make it harder for your to add tests later when
you want to, as there is more and more to do everytime you add something
to them.

Separating out functions to list the tests etc. could be useful, and
would mean that you could have simpler tests for what is actually
printed.

As for testing the commands themselves there are a couple of things you
can do. One is to pass in to the command a file object that they write
to, which is normally arranged to be sys.stdout, but can be a StringIO
or something in a test. The other is to have test methods that exec the
commands in a subprocess and capture the output. This is slower, but
gives you end-to-end coverage.

Some union of all three approaches would probably suit you as things
grow.

  * Funtions to do the work, separate from the UI classes. This allows
    you to test logic with targeted unit tests.

  * Tests of the command logic using test doubles for things like
    sys.stdout.

  * Tests that actually exec the commands and check that they did
    something. As these are the slowest and the hardest to debug you
    don't want to use them too much, but by this point there is little
    let to test except the fact that the command is available and
    actually does something, so they can be little more than smoke
    tests.

Your actual code looks fine to me, though things such as "walk_packages"
aren't immediately obvious to me, so I would have reached for tests to
ensure that it did what I thought.

Thanks,

James