Is having a switch to turn mocking on or off a code smell?

Question

I have a method that looks like this:

def foobar(mock=False, **kwargs):
    # ... snipped

foobar actually makes several calls to Amazon S3 and returns a composed result. In order to make this testable, I introduced the mock parameter to turn off making live network connections. It feels like a code smell for me but testability is also very important. What else can I do if I want to do away with the parameter ?

Answer 1

Why not have a connection class instead?

class Connection(object):
    def retrieve(self, resource):
        return something_from_s3()

class MockConnection(Connection):
    def retrieve(self, resource):
        return 42

def foobar(connection = Connection(), **kwargs):
    whatever = connection.retrieve("foobar")

Not only is this cleaner, but you can trivially test it with various mock connections. If you ever decide to support a different kind of service instead of S3, you can easily extend your product to support it.

Answer 2

Yes, that's a huge, HUGE code smell.

If you want to test something that's backed by S3, stub out or mock any calls you make to your S3 API and do it that way.

For a simple example, have a look at Paperclip: https://github.com/thoughtbot/paperclip/blob/master/test/storage_test.rb

Answer 3

Sometimes, it's easier to just pass some "s3 engine" as a parameter and pass some simple mock-object instead when it's a test (and provide it's return values). Sometimes you need to call lots of random functions that cause side-effects (for example, if you would work with s3, get current time, do some sql-queries, open some files and more), and here it wouldn't be nice to pass it all as function-parameters (since it isn't needed except for testability), I'd suggest you to use mockstar to declaratively describe your side-effects. Your code would then something like this:

def foobar():
    foo = get_from_database()
    bar = read_from_file()
    baz = read_from_s3()

    if foo > 10:
        return foo + bar + baz
    else:
        return foo - bar - baz

# And your test would look something like this:

from mockstar import prefixed_p
from nose.tools import assert_equal

ppatch = prefixed_p('module.with.foobar.func')


class TestFoobar(BaseTestCase):
    @ppatch('get_from_database')
    @ppatch('read_from_file')
    @ppatch('read_from_s3')
    def side_effects(self, se):
        se.read_from_file.return_value = 10  # default behaviour
        se.read_from_s3.return_value = 0  # also default behaviour
        return self.invoke(se)

    def test_should_get_30(self, se):
        se.get_from_database.return_value = 20

        # do
        result = foobar()

        assert_equal(result, 30)

    def test_should_get_minus5(self, se):
        se.get_from_database.return_value = 5

        # do
        result = foobar()

        assert_equal(result, -5)