Skip to content

chore: improve dagster maintenance path#33884

Open
lphuc2250gma wants to merge 1 commit into
dagster-io:masterfrom
lphuc2250gma:maint/20260527171823
Open

chore: improve dagster maintenance path#33884
lphuc2250gma wants to merge 1 commit into
dagster-io:masterfrom
lphuc2250gma:maint/20260527171823

Conversation

@lphuc2250gma

Copy link
Copy Markdown

Summary:

  • Add or tighten focused edge-case tests or type assertions in examples/deploy_ecs/tests/test_deploy.py, helm/dagster/schema/schema_tests/utils.py related to Python typing, tests, CLI ergonomics, observability; avoid docs-only changes and broad refactors.
  • Keep the change narrow so it is straightforward to review.

Notes:

  • I kept this scoped to the relevant implementation and tests.

@greptile-apps

greptile-apps Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR replaces two subprocess.call invocations with subprocess.run(..., check=True) in the docker_context pytest fixture, so that command failures are surfaced as exceptions instead of being silently ignored.

  • Adding check=True to docker context create is correct: if context creation fails the fixture should abort rather than proceed with a broken environment.
  • Adding check=True to docker context rm in the teardown (after yield) is risky: a non-zero exit during cleanup raises CalledProcessError at teardown time, which pytest surfaces as a fixture ERROR rather than the original test result, potentially masking the actual failure being debugged.

Confidence Score: 3/5

The teardown call gains check=True, which can turn a cleanup failure into a fixture ERROR that obscures the actual test outcome; warrants a fix before merging.

Using check=True in the fixture's teardown path changes how cleanup errors propagate: a non-zero exit from docker context rm now raises an exception during teardown rather than being silently ignored, potentially hiding the real reason a test failed. The setup-side change (docker context create with check=True) is sound, but the teardown-side change introduces a real behavioral regression in how pytest reports failures for this fixture.

examples/deploy_ecs/tests/test_deploy.py — specifically the docker context rm line in the fixture teardown.

Important Files Changed

Filename Overview
examples/deploy_ecs/tests/test_deploy.py Replaces subprocess.call with subprocess.run(..., check=True) in the docker_context fixture; check=True on the teardown (rm) call can raise CalledProcessError during cleanup, masking the actual test outcome.

Sequence Diagram

sequenceDiagram
    participant pytest
    participant docker_context fixture
    participant subprocess

    pytest->>docker_context fixture: setup
    docker_context fixture->>subprocess: run(["docker","context","create",...], check=True)
    alt create fails (non-zero exit)
        subprocess-->>docker_context fixture: CalledProcessError
        docker_context fixture-->>pytest: fixture setup ERROR (teardown skipped)
    else create succeeds
        subprocess-->>docker_context fixture: CompletedProcess(returncode=0)
        docker_context fixture-->>pytest: yield test_id
        pytest->>pytest: run test (xfail)
        pytest->>docker_context fixture: teardown
        docker_context fixture->>subprocess: run(["docker","context","rm",...], check=True)
        alt rm fails (non-zero exit)
            subprocess-->>docker_context fixture: CalledProcessError
            docker_context fixture-->>pytest: fixture teardown ERROR (masks original test result)
        else rm succeeds
            subprocess-->>docker_context fixture: CompletedProcess(returncode=0)
            docker_context fixture-->>pytest: teardown complete
        end
    end
Loading

Reviews (1): Last reviewed commit: "chore: improve dagster maintenance path" | Re-trigger Greptile

yield test_id

subprocess.call(["docker", "context", "rm", test_id])
subprocess.run(["docker", "context", "rm", test_id], check=True)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 check=True in fixture teardown can mask real failures

Adding check=True to the cleanup call means a non-zero exit from docker context rm raises CalledProcessError during teardown, converting a normal test failure (or xfail) into a fixture ERROR. If the context can't be removed — e.g., because a prior test run left state behind and the test_id fixture still collides, or because the ECS local-simulation container is still running when teardown fires — the exception surfaces instead of (or alongside) the real test outcome. Teardown code should generally suppress or log errors rather than propagate them, so that cleanup failures don't obscure the actual failure being investigated.

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.

1 participant