2

I'm working on an open-source test framework.

90% of my codebase has good test coverage. My main problem area is the command-line entry point. This module began life as a very short script for calling into the domain model (see the call to _run_impl()) and exit with code 1 if the test run failed.

It didn't feel necessary to test that script at the time that I wrote it, but as the project has sprouted features, a number of small changes to the logic in this file have led it to become quite long:

import argparse
import os
import sys
from . import _run_impl  # a function, in __init__.py, which calls into the domain model
from . import reporting  # a sub-package concerned with writing output to the console

import colorama; colorama.init()

def cmd():
    parser = argparse.ArgumentParser()
    parser.add_argument('-s', '--no-capture',
        action='store_false',
        dest='capture',
        default=True,
        help="Disable capturing of stdout during tests.")
    parser.add_argument('-v', '--verbose',
        action='store_true',
        dest='verbose',
        default=False,
        help="Enable verbose progress reporting.")
    parser.add_argument('--teamcity',
        action='store_true',
        dest='teamcity',
        default=False,
        help="Enable teamcity test reporting.")
    parser.add_argument('--no-random',
        action='store_false',
        dest='shuffle',
        default=True,
        help="Disable test order randomisation.")
    parser.add_argument('path',
        action='store',
        nargs='?',
        default=os.getcwd(),
        help="Path to the test file or directory to run. (default current directory)")
    args = parser.parse_args()

    if args.teamcity or "TEAMCITY_VERSION" in os.environ:
        reporters = (reporting.teamcity.TeamCityReporter(sys.stdout),)
    elif args.verbose:
        reporters = (reporting.cli.ColouredReporter(sys.stdout),)
    elif args.capture:
        reporters = (
            reporting.cli.DotsReporter(sys.stdout),
            type("ColouredCapturingReporter", (reporting.cli.ColouredReporter, reporting.cli.StdOutCapturingReporter), {})(sys.stdout),
            reporting.cli.TimedReporter(sys.stdout)
        )
    else:
        reporters = (
            reporting.cli.DotsReporter(sys.stdout),
            type("ColouredCapturingReporter", (reporting.cli.ColouredReporter, reporting.cli.SummarisingReporter), {})(sys.stdout),
            reporting.cli.TimedReporter(sys.stdout)
        )

    reporter = reporting.shared.ReporterManager(*reporters)

    _run_impl(os.path.realpath(args.path), reporter, args.shuffle)

    if reporter.failed:
        sys.exit(1)
    sys.exit(0)


if __name__ == "__main__":
    cmd()

I know that this code has bugs (for example, it should be possible to capture stdout from tests when running in verbose mode), and I've lost confidence in my ability to change this module without breaking something. So I want to write some tests for it - but I only really have experience with test-first development, so I don't really know where to start.

Should the tests be at the unit or integration level? Should I mock the ArgumentParser? Should I mock the reporting module? Should I mock _run_impl? Should I try to bring the whole module under test at the start, or just add tests as I add features? Do I need to refactor to make this method more testable?

Addendum: I think a part of my confusion is due to the fact that this is a relatively thin layer on top of the business logic - it's just there to make it possible to kick-off the test runner from the command line. So I'm feeling confused about what constitutes the service boundary here - in particular, what I should and shouldn't mock.

migrated from codereview.stackexchange.com Dec 2 '13 at 19:05

This question came from our site for peer programmer code reviews.

  • Apologies if this post is not appropriate for this board (too opinion-based? Not enough information about the rest of the code-base?). I thought about posting it on Programmers or Stack Overflow but I wasn't sure where it belonged. – Benjamin Hodgson Dec 1 '13 at 22:00
  • 1
    This question appears to be off-topic because it contains broken code. This looks like a possible good question for Programmers.StackExchange. – Mathieu Guindon Dec 2 '13 at 13:42
  • @retailcoder The code is not broken, it's just out of context, so you can't see the functions defined in other files. But I can't exactly post a whole project consisting of 1000 lines across 10 files here. (I did link to the Github repo in the question, if you want to see it in context.) I do agree that the question might be a better fit for Programmers though - how would I go about getting it moved? – Benjamin Hodgson Dec 2 '13 at 18:14
  • You can try flagging it for moderator attention with a custom flag :) – Mathieu Guindon Dec 2 '13 at 18:22
2

It seems like the main work is all outside this code. so tests for this code merely need to demonstrate that the calls it will perform are going to be performed reliably . Possibly you could also test that all of the 'reporters' lists will instantiate valid Reporter objects. This is all a perfectly legit case for plain old unit testing.

You can certainly break the code up into smaller bits suitable for unit testing -- for example, the argument parsing could become a function which would be fairly easy to test for different combinations of flags. It seems like the main job of this code is just to fill out the reporters list, so you should make sure that arguments produce the expected lists deterministically. Increasing testability might also suggest a refactor in which "reporters" becomes a class rather than a list so you can understand it's contents in a structured way -- that will be more testable and also perhaps more maintainable down the road, though it's hard to tell without knowing the APIs

Overall though this seems like a fairly testable scenario, and (like a lot of UI code ) one which would benefit from a battery of tests that make sure things like poorly formed or inconsistent arguments are handled in a reliable way.

4

Testing is usually hard when several ideas are coupled together in the same code; it means testing a lot of things at the same time. Breaking code apart into smaller chunks usually solves that problem.

In that vein, I would rewrite this slightly by extracting some methods:

def cmd(arguments):
    args = parse(arguments)
    reporter = create_reporter(args)

    _run_impl(os.path.realpath(args.path), reporter, args.shuffle)

   return not reporter.failed

if __name__ == "__main__":
   if cmd(sys.argv)
       sys.exit(0)
   sys.exit(1)

This has several benefits:

  • The code in cmd is not coupled to sys.argv and sys.exit(), which are both hard to deal with in a unit testing context
  • cmd is now simple to test if you mock out parse, create_reporter, and _run_impl
  • parse, create_reporter are simple and isolated enough to test easily on their own.

Python is not my primary language, so I can't recall if you'll need to create some classes to enable clean mocking, but if so do it. It's just a little bit of typing for a lot of value.

  • I'm a little reluctant to adopt your approach of extracting parse() and create_reporter() and testing them separately, because it couples the tests to the implementation of the module. For example, if I want to move a piece of logic from create_reporter() to parse() I have to rewrite my tests for both methods, which is bad because they are not part of the public API (they're just implementation details). I should be able to move non-public code around freely without breaking tests. – Benjamin Hodgson Dec 3 '13 at 10:33
  • 1
    "I should be able to move non-public code around freely without breaking tests." The down sides to that approach are many. First, if the parse responsibility starts throwing exceptions, your tests of the create_reporter responsibility also breaks. Second, your tests become harder to create and maintain -- to test creating a reporter, you have to create the right set of strings to get args to parse correctly. If you change an impl detail on what strings are allowed there, your reporter creation tests break. – tallseth Dec 3 '13 at 17:42
  • The real testability problem here is that the responsibility of parsing arguments is couple to the responsibility of creating reporters and executing tests. Fundamentally, you are saying that code that isn't public should be coupled together as much as you want, with no separation of responsibilities. – tallseth Dec 3 '13 at 17:43
  • I'm not saying that. I'm saying that I should be able to refactor the code's internals without breaking any tests. So could that isn't public could be highly coupled with no separation of responsibilities (I could implement the whole app as one long procedure if I wanted) or it could be SOLID object-oriented code. As long as the externally-observable behaviour is the same, I can refactor from one extreme to the other without breaking any tests. – Benjamin Hodgson Dec 3 '13 at 18:58
  • My point is that the tests should be just another consumer of the public API, and tell me nothing about the internal design of the code (in this case, the internal design would be the two extracted methods parse and create_reporter). – Benjamin Hodgson Dec 3 '13 at 19:01
3

I think you should test the command-line interface as part of integration testing.

Change your main function to:

def cmd(args=None):
    ...
    args = parser.parse_args(args=args)
    ...

...so that you can easily pass in a list of arguments in your tests.

If running the entire program during testing is unreasonable, mock out _run_impl.

You might also want to look at something like scripttest to help capture output.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.