Code review comment for lp:~frankban/lpsetup/commands-unittests

Revision history for this message
Benji York (benji) wrote :

This branch is really good. These tests will help us quite a bit while we
implement the LEP. Here are my thoughts from reading through the diff:

The combination of the negative test for step_name ("not in steps_to_skip") and
the ternary operator in _include_step is really hard for me to understand.
Maybe this instead:

def _include_step(self, step_name, namespace):
    """Return True if the given *step_name* must be run, False otherwise.

    A step is included in the command execution if the step included in
    `--steps` (or steps is None) and is not included in `--skip-steps`.
    """
    steps_to_skip = namespace.skip_steps or []
    steps_to_run = namespace.steps
    # If explicitly told to skip a step, then skip it.
    if step_name in steps_to_skip:
        return False
    # If no list of steps was provided then any non-skipped are to be run.
    if steps_to_run is None:
        return True
    # A list of steps to run was provided, so the step has to be included in
    # order to be run.
    return step_name in steps_to_run

I am not entirely sure the above is right, which bolsters my feeling that the
original was hard to understand.

Since the subcommand tests always have a get_arguments function which is called
and assigned to expected_arguments, it might be slightly better to just have a
required get_arguments method instead.

I don't feel strongly about it, but for the sake of diversity: here is a
version of get_random_string that is a bit shorter and more direct:

def get_random_string(size=10):
    """Return a random string to be used in tests."""
    return ''.join(random.sample(string.ascii_letters, size))

I really like the explanation in the StepsBasedSubCommandTestMixin docstring of
how to use the class. I wonder if it would be good to leave the "must define"
attributes undefined so AttributeErrors are generated if a subclass leaves them
out.

« Back to merge proposal