fix(lang): include mobile and skip refs in step dependency graph#20
Merged
Conversation
StepDependencies only walked When, Request, Expect, Capture and Keyword
expressions to extract implicit `result.<step>` references for the DAG.
It never traversed `step.Mobile` nor `step.SkipRules`, so a mobile step
referencing a previous keyword/HTTP step (e.g.
`input_text { value = result.user.email }`) had no recorded dependency
on `user`. The DAG placed both steps in the same topological layer and
ran them in parallel; the mobile evaluator then observed `result.user`
as the empty object pre-populated by NewScenarioState and failed with
the misleading HCL error "Unsupported attribute" in ~100us, before the
keyword had a chance to write its outputs. The same latent bug applied
to skip rules referencing other steps' results.
Refactor StepDependencies into focused helpers (collectRequestRefs,
collectExpectRefs, collectMobileRefs, collectMobileExpectRefs,
collectSkipRefs) to keep gocyclo under the v2 lint threshold while
covering every Expression in MobileStep (Platform, Target,
Launch.ClearState, each Action's five fields, every Expect.* slice
with their ID/Expected/Timeout/Interval fields) and every Expression
in SkipRule (Condition, Reason, OS, Arch, EnvSet, Env).
Adds four unit tests in refs_test.go for the new walks and a
DAG-level integration test in runner_test.go that builds a keyword
step followed by a mobile step referencing it and asserts buildLayers
produces two distinct layers in the expected order.
There was a problem hiding this comment.
Pull request overview
This PR fixes step dependency graph construction by ensuring StepDependencies also traverses expressions inside step.Mobile and step.SkipRules, so result.<step> references in those blocks correctly enforce execution ordering (preventing unintended parallel execution and misleading early HCL evaluation failures).
Changes:
- Refactors
StepDependenciesinto helper walkers for request/expect/mobile/skip rule expressions and adds the missing traversals. - Adds unit tests covering mobile action refs, mobile expect refs, mobile platform/launch refs, and skip rule refs.
- Adds an integration-style test asserting
buildLayersorders a mobile step after a keyword step it references viaresult.user.*.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/lang/refs.go | Adds request/expect/mobile/skip expression walkers so result.<step> refs are included in step DAG dependencies. |
| internal/lang/refs_test.go | Adds unit tests ensuring mobile and skip-rule expressions contribute implicit step dependencies. |
| internal/runtime/runner_test.go | Adds a DAG layering test proving mobile steps referencing prior steps are scheduled in later layers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
StepDependencies only walked When, Request, Expect, Capture and Keyword expressions to extract implicit
result.<step>references for the DAG. It never traversedstep.Mobilenorstep.SkipRules, so a mobile step referencing a previous keyword/HTTP step (e.g.input_text { value = result.user.email }) had no recorded dependency onuser. The DAG placed both steps in the same topological layer and ran them in parallel; the mobile evaluator then observedresult.useras the empty object pre-populated by NewScenarioState and failed with the misleading HCL error "Unsupported attribute" in ~100us, before the keyword had a chance to write its outputs. The same latent bug applied to skip rules referencing other steps' results.Refactor StepDependencies into focused helpers (collectRequestRefs, collectExpectRefs, collectMobileRefs, collectMobileExpectRefs, collectSkipRefs) to keep gocyclo under the v2 lint threshold while covering every Expression in MobileStep (Platform, Target, Launch.ClearState, each Action's five fields, every Expect.* slice with their ID/Expected/Timeout/Interval fields) and every Expression in SkipRule (Condition, Reason, OS, Arch, EnvSet, Env).
Adds four unit tests in refs_test.go for the new walks and a DAG-level integration test in runner_test.go that builds a keyword step followed by a mobile step referencing it and asserts buildLayers produces two distinct layers in the expected order.