test: refactor test_charm.py to pytest fixtures and parametrize

[IronCore864]

Refactoring test_charm.py to pytest style.

---

This PR contains a few major changes:

• a fake script helper fixture in pytest style to decouple the test case with the fake script functions
• use pytest's parametrize instead of a for loop
• use fixtures to setup/cleanup

Note that this PR might show more changes because it's checked out based on the previous unmerged PR here.

---

1 Fake Script Helper Fixture

Background: Previously, fake_script_path is set on the TestCase itself, then self is passed into the fake script functions, and self.addCleanup() is added on self. The test case is mixed with fake script, fake script adds clean up to the test case, making it hard to convert to pytest's fixture-style cleanup.

Changes: In these two commits, a new class FakeScriptFixture is created to share fake_script_path between the two methods fake_script() and fake_script_calls.

After the change, we can use it like:

@pytest.fixture
def fake_script_fixture(monkeypatch: MonkeyPatch, tmp_path: pathlib.Path):
    return FakeScriptFixture(monkeypatch, tmp_path)

This fixture's purpose is to add another layer on top of the FakeScriptFixture class so that we don't have to pass monkeypatch/tmp_path fixtures to it everytime we use it. Then, in the test case, we can do something like:

def test_foo(fake_script_fixture):
    fake_script_fixture.fake_script("test", "echo 'hello'")

---

2 Use Parametrize/Fixture to Refactor test_charm.py

• Tests no longer inherit from unittest.TestCase.
• Use fixture to create framework and teardown.
• Use custom marker (added to pyproject.toml to depress unrecognized warning) to pass data to the framework fixture so that it can be customized.

Still in draft, please review and merge PR 1192 first so that this PR shows less change. I'll do a final review tomorrow.

[benhoyt]

I really like the fixture for the fake_script thing, but I'm not loving how the charm_meta / framework thing works out here. Let's discuss some of this further at our daily sync.

[IronCore864]

Many comments are interrelated, so I will reply to all of them here in a single comment to take some notes of my thoughts:

Autouse

The original test case has the env and events mocking defined in the setup function, meaning they are applied to all test cases. Autouse is exactly the same, except instead of defining code in a setup function, you define it in a fixture with autouse=True. So, to me, this change makes sense.

I think it's a solid point that if the users are not familiar with the autouse function, it's easy to go wrong since it's applied on all test cases without explicitly saying so. So I think maybe we should not use autouse at all.

This creates a minor issue where you have the fixture in the test function's parameter but you don't use it:

@pytest.fixture
def env(monkeypatch):
    monkeypatch.setenv("FOO", "BAR")
    yield


def test_foo(env):  # 
    # do something but "env" is not accessed, showing up in IDE prompts

Reasons for 3 Fixtures Instead of Only One

If we get rid of autouse and we don't want the "not accessed", we can combine the three fixtures into one, but I think it loses some readability. By splitting the original setUp (and the create_framework func) into 3 fixtures, it is more readable, because:

• you get three short simple functions that all have the keyword yield, before which is the setup and after which is the teardown
• each function does one thing and one thing only

@pytest.fixture(autouse=True)
def env(monkeypatch):
    monkeypatch.setenv(xxx)
    yield


@pytest.fixture(autouse=True)
def events():
    ops.CharmBase.on = "some mock"
    yield
    ops.CharmBase.on = "change back to original value"


@pytest.fixture
def framework(request: pytest.FixtureRequest, tmp_path: pathlib.Path):
    framework = ops.Framework(xxx)
    yield framework
    framework.close()

By reading this piece of code, it can be easily inferred that all test cases do the three following things:

• mock env
• mock events
• create a framework object

If we combine them into one, for example:

@pytest.fixture
def framework(
    monkeypatch: pytest.MonkeyPatch,
    request: pytest.FixtureRequest,
    tmp_path: pathlib.Path
):
    monkeypatch.setenv("PATH", str(Path(__file__).parent / 'bin'), prepend=os.pathsep)
    monkeypatch.setenv("JUJU_UNIT_NAME", "local/0")

    class CustomEvent(ops.EventBase):
        pass

    class TestCharmEvents(ops.CharmEvents):
        custom = ops.EventSource(CustomEvent)

    # Relations events are defined dynamically and modify the class attributes.
    # We use a subclass temporarily to prevent these side effects from leaking.
    ops.CharmBase.on = TestCharmEvents()  # type: ignore

    marker = request.node.get_closest_marker("charm_meta")  # type: ignore
    meta = marker.args[0] if marker else ops.CharmMeta()  # type: ignore
    model = ops.Model(meta, _ModelBackend('local/0'))  # type: ignore
    # we can pass foo_event as event_name because we're not actually testing dispatch
    framework = ops.Framework(SQLiteStorage(':memory:'), tmp_path, meta, model)  # type: ignore

    yield framework

    ops.CharmBase.on = ops.CharmEvents()  # type: ignore
    framework.close()

It's still short enough to be readable, but I think it's less clear than 3 separate fixtures and harder to figure out the logic.

Create Framework as a Function, not a Fixture

I think we can keep the framework creation part as a function, just like before. Example:

def create_framework(self, request: pytest.FixtureRequest, tmp_path: pathlib.Path):
    monkeypatch.setenv("PATH", str(Path(__file__).parent / 'bin'), prepend=os.pathsep)
    monkeypatch.setenv("JUJU_UNIT_NAME", "local/0")

    class CustomEvent(ops.EventBase):
        pass

    class TestCharmEvents(ops.CharmEvents):
        custom = ops.EventSource(CustomEvent)

    ops.CharmBase.on = TestCharmEvents()

    def cleanup():
        ops.CharmBase.on = ops.CharmEvents()

    request.addfinalizer(cleanup)
    
    model = ops.Model(meta, _ModelBackend('local/0'))  # type: ignore
    framework = ops.Framework(SQLiteStorage(':memory:'), tmp_path, self.meta, model)  # type: ignore
    request.addfinalizer(framework.close)

    return framework

As much as I hate the multi-line custom marker just to pass data into the framework fixture, turning it into a function instead of a fixture isn't the perfect solution since:

• less readable
• every time you call it, you have to pass in two parameters: self.create_framework(request, tmp_path), and you have to include request, tmp_path in almost all test cases, like:

class TestClass:
    def test_foo(self, request, tmp_path):
        self.create_framework(request, tmp_path)

Compromise

Maybe a compomise is a mix of everything mentioned above?

import functools


@pytest.fixture(autouse=True)
def env(monkeypatch):
    monkeypatch.setenv(xxx)
    yield


@pytest.fixture(autouse=True)
def events():
    ops.CharmBase.on = "some mock"
    yield
    ops.CharmBase.on = "change back to original value"


class TestClass:
    @pytest.fixture
    def create_framework(request, tmp_path):
        return functools.partial(self._real_create_framework, request, tmp_path)    

    def _real_create_framework(self, request, tmp_path):
        model = ops.Model(meta, _ModelBackend('local/0'))
        framework = ops.Framework(SQLiteStorage(':memory:'), tmp_path, self.meta, model)
        request.addfinalizer(framework.close)
        return framework

    def test_foo(self, create_framework):
        self.meta = xxx
        framework = create_framework()

[IronCore864]

According to our discussion in the daily, we decide to:

• make create_framework a function, not a fixture: create_framework(request, *, meta=None)
• put it outside the test class (in test_helper.py maybe) for reusability
• remove tmp_path fixture so that we have one less fixture to pass to the function and we will do the tmp path creation/cleanup manually in the create_framework function
• remove autouse, which means no more env and events fixtures any more

I'll do a separate commit later to reflect this change and refactor the test cases accordingly.

[IronCore864]

According to our discussion, fake_script fixture will be defined in test_helpers.py, not conftest.py. However, there is already a function with the same name in test_helpers.py which is referred by other test files other than test_charm.py.

So, I will put the fake_script fixture in test_charm.py temporarily; after refactoring all test files, I will move it to test_helpers.py.

[benhoyt]

This is looking really good now, I think. Just some minor comments. By the way, the diff is much easier to view with "hide whitespace" turned on in the GitHub UI.

[tonyandrewmeyer]

Looking good! A few small suggestions.

[benhoyt]

Looks good to me, just a couple of minor styling comments.

[tonyandrewmeyer]

A couple of small suggestions (including Ben's ones) but otherwise looking good!

[tonyandrewmeyer]

LGTM!