Skip to content

MockVWS can be used as a decorator but carries state between calls #3390

Description

@adamtheturtle

What happens

MockVWS inherits from contextlib.ContextDecorator:

class MockVWS(ContextDecorator):

so an instance can be used as a decorator. The target manager is built in __init__ rather than in __enter__:

self._target_manager = TargetManager()

so every use of the same instance shares one set of databases and targets. Decorating two functions with one instance:

mock = MockVWS()
mock.add_cloud_database(cloud_database=db)

@mock
def add_one_target(): ...   # POST /targets with name "only-one"

@mock
def count_targets(): ...    # GET /targets

gives:

first call  add_one_target -> 201
targets now -> 1
second call add_one_target -> 403
targets now -> 1

The second call fails with TargetNameExist because the target from the first call is still there.

Why it matters

The decorator form is the one where per-call isolation is most expected, because that is what the comparable tools do — @responses.activate and @mock.patch both give each call a clean slate. Someone reaching for @MockVWS() on a set of test methods gets order-dependent tests and a 403 that does not obviously point at the cause.

ContextDecorator is not currently exercised or described anywhere: there is no use of MockVWS as a decorator in tests/, in docs/, or in README.rst. Every documented example uses with MockVWS() as mock:. So this is a public capability with no tests, no documentation, and a sharp edge.

I am not sure which way this should go, which is why this is a question rather than a bug report.

Suggested resolution

Pick one.

Make each use isolated. Move the TargetManager construction into __enter__. Every documented example adds its databases after entering the context, so this does not break the documented usage, and it makes @MockVWS() behave the way the decorator idiom implies. It does change behaviour for anyone relying on add_cloud_database before __enter__, which is a real pattern even if it is not the documented one, so it needs a changelog note.

Or document the current behaviour. State in the API reference that a MockVWS instance carries state across uses and that a fresh instance is wanted per test, and add a decorator example to the docs so the supported usage is visible.

Or drop ContextDecorator. If decorator use is not intended to be supported, inheriting from it advertises something the project does not want to promise, and removing it makes the context manager the single documented entry point.

Whichever is chosen, the decorator path wants a test, since it currently has none.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions