Skip to content

test: adapt StepScript de-sugaring tests to openjd-model 0.11.9 - #1085

Merged
seant-aws merged 1 commit into
aws-deadline:mainlinefrom
seant-aws:fix-stepscript-desugar-tests
Sep 9, 2026
Merged

seant-aws merged 1 commit into
aws-deadline:mainlinefrom
seant-aws:fix-stepscript-desugar-tests

Conversation

@seant-aws

Copy link
Copy Markdown
Contributor

What was the problem/requirement? (What/Why)

openjd-model 0.11.9 changed resolve_syntax_sugar() so that step-scope let is no longer folded into the produced script.let. Step-scope bindings now travel exclusively through the service-served resolvedSymbolTable. Two tests asserted the old fold behavior and broke when CI resolved 0.11.9 (Sep 4 2026).

What was the solution? (How)

Adapted the two failing tests to the new upstream contract:

  • test_start_de_sugars_a_bash_step: assert script.let carries only the simple-action let
  • test_step_and_sugar_scope_let_both_resolve: seed resolvedSymbolTable with the step-scope binding instead of relying on the fold

Updated docstrings in _resolve_step_script and the pyproject.toml dependency comment to reflect the new single-channel design.

What is the impact of this change?

Test-only. No production logic or dependency ranges changed.

How was this change tested?

Full unit matrix (3189 passed × 3 Python versions), lint clean (ruff + mypy + format).

Was this change documented?

Docstrings and dependency comments updated in-place.

Is this a breaking change?

No.

openjd-model 0.11.9 changed resolve_syntax_sugar() so that step-scope
`let` is no longer folded into the produced script.let. Step-scope
bindings now travel exclusively through the service-served
resolvedSymbolTable. The worker-agent production code already forwards
this table correctly; only tests and docstrings needed updating.

Changes:
- test_start_de_sugars_a_bash_step: assert script.let carries only the
  simple-action let, not the step-scope let
- test_step_and_sugar_scope_let_both_resolve: seed resolvedSymbolTable
  with the step-scope binding instead of relying on the fold
- _resolve_step_script docstring: reflect the new single-channel design
- pyproject.toml comment: note the 0.11.9 contract change

No production logic or dependency ranges changed.

Signed-off-by: Sean Tang <171081544+seant-aws@users.noreply.github.com>
@github-actions github-actions Bot added the waiting-on-maintainers Waiting on the maintainers to review. label Sep 9, 2026
@seant-aws
seant-aws marked this pull request as ready for review September 9, 2026 19:07
@seant-aws
seant-aws requested a review from a team as a code owner September 9, 2026 19:07
Comment thread pyproject.toml
# longer includes step-scope `let`, which now travels exclusively through the
# service-served resolvedSymbolTable. This package's tests have been adapted
# to that contract.
"openjd-model >= 0.11.4, < 0.12",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The dependency floor should be raised to >= 0.11.9 to match what this PR now relies on.

The comment directly above states that resolve_syntax_sugar()'s fold behaviour changed in openjd-model 0.11.9, and both the docstring in run_step_task.py and the assertions in test_run_step_task.py / test_step_scope_let_end_to_end.py now encode the post-0.11.9 contract. But the specifier still admits 0.11.4 through 0.11.8, where the fold still emits [*step lets, *simple-action lets]. Against any of those versions:

  • test_start_de_sugars_a_bash_step fails outright (step_script.let would be ["base = 'from step'", "msg = base"], not ["msg = base"]).
  • More importantly at runtime, step-scope names arrive on both channels again — resolved by the service in resolvedSymbolTable and as source expressions re-evaluated by the session — which is exactly the unverified double-declaration hazard the removed docstring paragraph described. The new docstring asserts "those names no longer arrive twice", which is false for a resolvable version in the current range.

Note this is the same shape of coupling the openjd-sessions == 0.12.0 pin just above documents ("this pin and the code change in this PR must ship together"). Suggest "openjd-model >= 0.11.9, < 0.12", and updating the comment to say the floor was raised for this reason rather than that no floor change is needed.

step_let=["base = 'from step'"],
sugar_let=["out = base + '|sugar'"],
script_body='echo "OUT:{{ out }}"',
resolved_symbol_table=[{"name": "base", "type": "string", "value": "from step"}],

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Adding resolved_symbol_table here removes the only coverage of a sugar template that has step-scope let but no served resolvedSymbolTable.

After this change, in TestSimpleActionSugarEndToEnd:

  • test_a_bash_sugar_step_runs_and_resolves_its_own_let passes step_let=None,
  • this test passes step_let=[...] plus a matching resolved table.

So nothing exercises step_let=["base = ...] with resolved_symbol_table=None. That combination used to work — the fold re-declared base in script.let — and under >=0.11.9 it now fails to resolve, because neither channel supplies base. The behaviour change is intentional per the docstring, but it means correctness on the sugar path is now entirely contingent on the service always populating resolvedSymbolTable whenever a step declares let. That assumption is worth pinning: either a negative test asserting the action fails cleanly in that case, or a note in the docstring that a served-table-less sugar template with step-scope let is out of contract.

For comparison, TestResolvedSymbolTableEndToEnd.test_a_step_without_resolved_table_still_runs deliberately keeps the no-table control for the script: path; the sugar path no longer has an equivalent for the let-bearing case.

@seant-aws
seant-aws merged commit 060709a into aws-deadline:mainline Sep 9, 2026
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

waiting-on-maintainers Waiting on the maintainers to review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants