Page MenuHomeElementl

Customize instance fixture
ClosedPublic

Authored by jordansanders on Jun 30 2021, 8:35 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Aug 9, 10:53 PM
Unknown Object (File)
Sat, Aug 6, 11:44 AM
Unknown Object (File)
Thu, Aug 4, 7:41 AM
Unknown Object (File)
Thu, Aug 4, 3:34 AM
Unknown Object (File)
Thu, Aug 4, 3:16 AM
Unknown Object (File)
Wed, Aug 3, 10:15 AM
Unknown Object (File)
Tue, Aug 2, 12:38 AM
Unknown Object (File)
Mon, Aug 1, 7:19 PM
Subscribers
None

Details

Summary

This will let us pass overrides for each test.

The instance fixture still provides our default instance:

def test_instance(instance):
    assert isinstance(instance.run_launcher, EcsRunLauncher)

But tests can also use the lower level instance_cm fixture to
customize the Dagster Instance (while still reaping the benefit of
ignoring the minutia of the stub_aws and metadata test setup):

def test_instance(instance_cm):
    with instance_cm({"container_name": "foo"}) as instance:
        assert isinstance(instance.run_launcher, EcsRunLauncher)
        assert instance.container_name == "foo"

The reason instance_cm returns a contextmanager rather than yielding
the instance directly is because when you yield a function that yields,
it returns a generator and not the thing being yielded. So callers would
need to call next(instance) to actually get their instance. Rough.

Another approach I looked at was using pytest parameterization:

@pytest.fixture
def instance_config():
    return {}

@pytest.fixture
def instance(stub_aws, metadata, instance_config):
    overrides = {
        "run_launcher": {
            "module": "dagster_aws.ecs",
            "class": "EcsRunLauncher",
            "config": {**(instance_config or {})},
        }
    }

    with instance_for_test(overrides) as instance:
        yield instance

def test_instance(instance):
    # If you don't parametrize, it uses the default `instance_config`.
    assert isinstance(instance.run_launcher, EcsRunLauncher)

@pytest.mark.parametrize("instance_config", [{"container_name": "foo"})
def test_instance_parametrized(instance):
    assert isinstance(instance.run_launcher, EcsRunLauncher)
    assert instance.container_name == "foo"

While perhaps better adhering to pytest idioms, it's an inelegent
solution for two reasons:

  1. You can't easily parametrize the same fixture multiple times. So a test that needs multiple instances - each with their own config - is diffult to achieve.
  2. There's not an easy way for the parameter itself to source from a fixture (like if instead of {"container_name": "foo"}, we wanted {"container_name": foo_fixture}. This means we cede control of the fixture setup/teardown chain because we can't make our instance fixture depend on another fixture.
Test Plan

unit

Diff Detail

Repository
R1 dagster
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

jordansanders created this revision.

pylint ignore unused-argument

dgibson added inline comments.
python_modules/libraries/dagster-aws/dagster_aws_tests/ecs_tests/test_launcher.py
90–93

this pattern confuses me / seems wrong at first glance? You have delayed cleaning up the tempdir, but you haven't delayed cleaning up the instance, which also does cleanup / shuts things down besides the tempdir. In general its not safe to use an instance (or a contextmanager object in general) after it has gone through its exit method, which seems like it will happen once the instance is returned

Maybe there's some fixture magic that i'm not seeing or misunderstanding?

This revision now requires changes to proceed.Jul 1 2021, 2:10 PM
jordansanders retitled this revision from Refactor instance fixture into an instance factory to Customize instance fixture.
jordansanders edited the summary of this revision. (Show Details)

Refactor to return a contextmanager

dgibson added inline comments.
python_modules/libraries/dagster-aws/dagster_aws_tests/ecs_tests/test_launcher.py
63

name's a bit vague, what kind of metadata?

This revision is now accepted and ready to land.Jul 1 2021, 7:00 PM
This revision was landed with ongoing or failed builds.Jul 2 2021, 9:18 PM
This revision was automatically updated to reflect the committed changes.