Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: introducing a fixture for proper teardown #242

Merged
merged 1 commit into from
Feb 15, 2024

Conversation

mmikita95
Copy link
Contributor

Looks quite hellish in the diff, but essentially this adds a fixture to properly initiate an AppRunner (with dynamic parameters), and to shut it down during test teardown, to address the issue with AppRunner not being shut down if the test fails.

@ramedina86
Copy link
Collaborator

Nice. Thanks for seeing something that's wrong and doing something about it.

Yes, looks like the diff is mainly the result of the tabs for the context managers.

@ramedina86 ramedina86 merged commit a9af5dd into writer:dev Feb 15, 2024
12 checks passed

from tests import test_app_dir


@pytest.fixture
def setup_app_runner():
Copy link
Collaborator

@FabienArcellier FabienArcellier Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would advocate to remove the dependence to pytest here, it's not required because we use it as a simple context manager. The boilerplate for pytest make that heavy and explicit dependency is better when we can.

with setup_app_runner("./not_an_app", "run") as ar:
  pass

Copy link
Collaborator

@FabienArcellier FabienArcellier Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and add the typing if you can

@contextlib.contextmanager
def setup_app_runner(app_dir: str, app_command: str):
    ar = AppRunner(app_dir, app_command)
    try:
        yield ar
    finally:
        ar.shut_down()

Copy link
Collaborator

@FabienArcellier FabienArcellier Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from conftest import setup_app_runner

def test_init_wrong_path(self) -> None:
    with setup_app_runner("./not_an_app", "run") as ar:
        with pytest.raises(SystemExit) as wrapped_e:
            ar.load()
        assert wrapped_e.type == SystemExit

@mmikita95 mmikita95 deleted the fix-app-runner-tests-teardown branch February 15, 2024 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants