Moto: Unexpected behavior with @mock_s3 for a test suite

[eliasdorneles]

Hey folks!

I was bitten by a weird behavior of moto today, I decided to take some time and build a test case for the behavior I'd expect:

import boto
import unittest
from moto import mock_s3


@mock_s3
class TestWithSetup(unittest.TestCase):
    def setUp(self):
        conn = boto.connect_s3()
        conn.create_bucket('mybucket')

    def test_should_find_bucket(self):
        conn = boto.connect_s3()
        self.assertIsNotNone(conn.get_bucket('mybucket'))

    def test_should_not_find_unknown_bucket(self):
        conn = boto.connect_s3()
        with self.assertRaises(Exception):
            conn.get_bucket('unknown_bucket')


@mock_s3
class TestWithPublicMethod(unittest.TestCase):
    def ensure_bucket_exists(self):
        conn = boto.connect_s3()
        conn.create_bucket('mybucket')

    def test_should_find_bucket(self):
        self.ensure_bucket_exists()

        conn = boto.connect_s3()
        self.assertIsNotNone(conn.get_bucket('mybucket'))

    def test_should_not_find_bucket(self):
        conn = boto.connect_s3()
        self.assertIsNone(conn.get_bucket('mybucket'))


@mock_s3
class TestWithPseudoPrivateMethod(unittest.TestCase):
    def _ensure_bucket_exists(self):
        conn = boto.connect_s3()
        conn.create_bucket('mybucket')

    def test_should_find_bucket(self):
        self._ensure_bucket_exists()
        conn = boto.connect_s3()
        self.assertIsNotNone(conn.get_bucket('mybucket'))

    def test_should_not_find_bucket(self):
        conn = boto.connect_s3()
        self.assertIsNone(conn.get_bucket('mybucket'))

This is related to commit https://github.com/spulec/moto/commit/3ed9428cb01cc0af157dfaf50fc4550e1a6d3ab5 for the issue https://github.com/spulec/moto/issues/363 -- I think this fix is not the best way, because it seems to be making the contents of the bucket global to the test suite, while the expected IMO would be to make it local to each test method.

Before that change, it seems that the contents of the bucket was indeed local to every method, but the result was unexpected because it was isolating also setUp and other non-test methods.

I'm not sure how to fix it, I hope the test case helps at least to decide which is the expected behavior.

[eliasdorneles]

As an additional note, the workaround to get the expected behavior in a test suite is to avoid the @mock_s3 decorator and control the mock manually:

class WorkaroundTest(unittest.TestCase):

    def setUp(self):
        self.mock_s3 = moto.mock_s3()
        self.mock_s3.start()
        self.populate_fixture_bucket()

    def populate_fixture_bucket():
        """populate bucket with test data here"""

    def tearDown(self):
        self.mock_s3.stop()

[spulec]

I agree that it is an ambiguous whether or not the class-based decorator should create a global state or a local state to each test. Any idea if there is any precedent for other class-based decorators? Mock?

[eliasdorneles]

@spulec right, I suppose @mock.patch is a good source of inspiration (it can be used as function decorator, class decorator or context manager). :+1:

It creates local state to each test, and it finds the test methods searching for a configurable prefix. Here is a snippet from the code comments:

Patch can be used as a TestCase class decorator. It works by
decorating each test method in the class. This reduces the boilerplate
code when your test methods share a common patchings set. patch finds
tests by looking for method names that start with patch.TEST_PREFIX.
By default this is test, which matches the way unittest finds tests.
You can specify an alternative prefix by setting patch.TEST_PREFIX.

Source: https://github.com/testing-cabal/mock/blob/27a20329b25c8de200a8964ed5dd7762322e91f6/mock/mock.py#L1646

[spulec]

Okay, I agree we should change it.

I can't seem to think of anyway to do this without special-casing 'setUp' though. From my reading of the mock code, it seems that they don't patch during the setUp, though I think that is probably something we do want.

Thoughts?

[suederade]

Has this been changed?