test(e2e): fix mypy Optional narrowing in test_session_runtime - #1046
Conversation
Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>
Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>
| interval=10, | ||
| ) | ||
| def wait_worker_started() -> None: | ||
| assert worker.worker_id is not None # narrow Optional inside the closure for mypy |
There was a problem hiding this comment.
This narrowing assert now lives inside a closure decorated with @backoff.on_exception(backoff.constant, Exception, max_time=120, interval=10), so it is caught and retried like a transient failure. If worker.worker_id were ever None, this would burn the full 120s retry window and then surface as a bare AssertionError with no message, rather than failing fast with a clear cause.
Note also that line 531 already does assert worker.worker_id is not None immediately above — mypy just does not carry narrowing of a member expression into a nested function body, which is why the duplicate is needed here.
A cleaner fix that satisfies mypy without putting a non-retryable precondition inside the retry loop is to bind a local before the closure:
worker_id = worker.worker_id
assert worker_id is not None
@backoff.on_exception(backoff.constant, Exception, max_time=120, interval=10)
def wait_worker_started() -> None:
assert is_worker_started(
deadline_client=deadline_client,
farm_id=deadline_resources.farm.id,
fleet_id=deadline_resources.fleet.id,
worker_id=worker_id,
)A local variable stays narrowed inside the closure, so the assert is checked once, up front, and the duplication on line 531 goes away.
| - name: Set up Python | ||
| uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 | ||
| with: | ||
| python-version: '3.11' |
There was a problem hiding this comment.
Pinning this gate to a single interpreter means the e2e type check only ever runs one of the two mypy tracks the repo supports. requirements-e2e.txt splits on interpreter version:
mypy >= 2.3.0, < 2.4; python_version >= "3.10"
mypy >= 1.19.1, < 1.20; python_version < "3.10"
so CI will only exercise mypy 2.3 under 3.11, while a contributor on 3.9 gets mypy 1.19 locally. Separately, test/e2e/mypy.ini sets no python_version, so mypy infers 3.11 from the running interpreter — version-conditional typing errors under the project minimum (requires-python = ">=3.9") will not be caught here.
Two options depending on intent: either add python_version = 3.9 to test/e2e/mypy.ini so the check is pinned to the floor regardless of which interpreter CI happens to use, or give this job the same python-version matrix as UnitTest/IntegrationTests above so both mypy tracks are covered. The former is cheaper and probably sufficient; the ruff half of e2e:lint is version-independent either way.
What was the problem/requirement? (What/Why)
hatch run e2e:lint(thee2ehatch env's ruff + mypy overtest/e2e) is a separate lint gate that CI never ran, so a mypy error in the e2e tests reached mainline and broke the release publish step. The standardhatch run lintin CI coverssrc/test/unit, nottest/e2e.What was the solution? (How)
Two changes:
test/e2e/test_session_runtime.pypassedworker.worker_id(str | None) tois_worker_started(worker_id: str). It is narrowed with anassert ... is not Nonein the enclosing scope, but the call sits inside thewait_worker_started()backoff closure, where mypy re-widens the Optional. Added the assert inside the closure, matching the existing idiom intest_worker_status.py.e2e:lintin CI. Added anE2E Lintjob to the Code Quality workflow that runshatch run e2e:linton every PR and mainline push, so this class of error is caught going forward. The job only lints/type-checks (no AWS credentials, no live e2e run).What is the impact of this change?
Test/CI-only. No runtime or shipped-code change. PRs now run ruff + mypy over
test/e2e.How was this change tested?
hatch run e2e:lintlocally:ruff check"All checks passed",ruff format --check"15 files already formatted",mypy"Success: no issues found in 15 source files". Workflow YAML validated (parses; jobs: UnitTest, IntegrationTests, E2ELint). The new job also runs on this PR.Was this change documented?
No; test/CI-only.
Is this a breaking change?
No.