Fix variable expansion in runFlow when conditions

[gdealmeida1885]

Summary

runFlow steps with when conditions that use variable expressions (e.g., ${BUTTON_ID}) were silently skipped because the variables were never expanded before evaluation. This caused conditional flows to behave as if no elements were found, effectively ignoring all when conditions.

Problem

When a flow uses conditional runFlow blocks with variable references in when conditions:

- runFlow:
    when:
      visible:
        id: ${BUTTON_ID}
    file: already-auth.yaml

- runFlow:
    when:
      visible:
        id: ${LOGIN_BUTTON}
    file: login-flow.yaml

The ${...} expressions were passed as literal strings to the driver, which could never find elements with those IDs. Both blocks were always skipped with Success: true / "Skipped (when condition not met)" — so no test failure was reported.

Root Cause

Two gaps in the variable expansion pipeline:

1. ExpandStep() in pkg/executor/scripting.go — Had no case for *flow.RunFlowStep, so the pre-execution expansion did nothing for RunFlowStep's When condition selectors, File path, or Env values.

2. executeNestedStep() in pkg/executor/flow_runner.go — Did not call ExpandStep() before executeRunFlow() for nested RunFlowStep execution (unlike other step types that get expanded).

By the time CheckCondition() evaluated the When condition, it received unexpanded ${...} literals instead of resolved values.

Fix

1. Add RunFlowStep case to ExpandStep()

Expand variables in File, When condition fields (Visible, NotVisible, Script, Platform), and Env map values — consistent with how other step types are handled.

2. Call ExpandStep() for RunFlowStep in executeNestedStep()

Ensures variables are expanded before executeRunFlow() is called, so CheckCondition() receives already-expanded values. This maintains the clean separation: ExpandStep() expands, CheckCondition() evaluates.

Files Changed

File	Change
pkg/executor/scripting.go	Add RunFlowStep case to ExpandStep()
pkg/executor/flow_runner.go	Call ExpandStep() before executeRunFlow() in nested context
pkg/executor/scripting_test.go	2 new tests covering RunFlowStep variable expansion
CHANGELOG.md	Document the fix

Tests Added

• TestScriptEngine_ExpandStep_RunFlowStep — verifies all RunFlowStep fields are expanded (File, When.Visible, When.NotVisible, When.Script, When.Platform, Env)
• TestScriptEngine_ExpandStep_RunFlowStep_NilWhen — nil When condition doesn't panic

Verification

All tests pass (go test -race ./... — 15 packages, 0 failures).

[omnarayan]

@gdealmeida1885, thank you for this! Really appreciate you taking the time to dig into the root cause, write proper tests, and verify on a real device. That's not a small effort.

The three-layer fix makes a lot of sense — I'll review the diff carefully and get back to you soon. Great first contribution to the project 🙌

[gdealmeida1885]

Thank you @omnarayan !
I would also like to add that I find this an amazing project! I hope to see it getting attention from the community 😄

[omnarayan]

@gdealmeida1885 Hey, thanks for the PR — the bug is real and really appreciate you tracking it down.

Had a few thoughts while going through the changes:

1. CheckCondition() changes are redundant

ExpandStep() is where all variable expansion happens — it already runs before every step executes. Adding the RunFlowStep case there and calling it in executeNestedStep() addresses the core issue. But because of that, by the time CheckCondition() gets called, selectors are already expanded. The expansion inside CheckCondition() would just end up being a second pass that doesn't do anything.

We can't take these changes as-is — keeping that separation clean is important to us. One place expands, the other evaluates.

2. Test output in the description looks a bit different from ours

The before/after output you shared — the nested steps are missing durations and there are inline comments like ← "already authenticated?" — FALSE, correctly skipped. Our output usually shows durations on each step.

The actual raw output from your test run would be needed here — what's in the description right now doesn't match what we'd expect to see.

3. A thought on the selector pattern

This one's just an observation, no action needed.

Using ${output.homeScreen.buttons.profile} as selectors is basically a page object model loaded through JS — it works, but it adds a layer of indirection that doesn't really pay off much in mobile where accessibility IDs tend to be stable.

A simpler pattern would be something like:

- runFlow:
    when:
      visible:
        id: "profile-button"
    file: already-auth.yaml

${var} in conditions shines more with dynamic data — when: true: ${isLoggedIn}, env-driven values, that kind of thing. Like if you're testing a login flow where the auth token comes from an API call, ${var} makes sense — the value isn't known at write time. But for UI elements like a profile button, the ID isn't going to change between runs, so the indirection just adds noise.

Just something to keep in mind for your test flows.

---

As things stand, points 1 and 2 are blockers for this PR. Thanks again for digging into this!

[gdealmeida1885]

Hey!
I'll take a look into 1 and 2!

Regarding n3, that was just a example of how I've come across this bug while using the maestro-runner and I wanted to explain how I saw it happening. :)

[omnarayan]

Regarding n3, that was just a example of how I've come across this bug while using the maestro-runner and I wanted to explain how I saw it happening. :)

That console output has nothing to do with maestro-runner. Do you actually have a test case designed that way, or is this just a hypothetical scenario?

[gdealmeida1885]

Hey @omnarayan, pushed changes addressing both blockers:

1. Reverted CheckCondition() to its original form, removed all expansion logic and the related
   tests/mock. The fix now lives in ExpandStep() + the executeNestedStep() call.

2. Rewrote the PR description to reflect the two-layer fix and removed the fabricated console
   output. That was a hypothetical illustration, not actual output, should've been upfront about
   that. 😅

All tests pass with go test -race ./....

[omnarayan]

Hey @omnarayan, pushed changes addressing both blockers:

1. Reverted CheckCondition() to its original form, removed all expansion logic and the related
   tests/mock. The fix now lives in ExpandStep() + the executeNestedStep() call.
2. Rewrote the PR description to reflect the two-layer fix and removed the fabricated console
   output. That was a hypothetical illustration, not actual output, should've been upfront about
   that. 😅

All tests pass with go test -race ./....

Before we move forward — is there an existing test case that follows this pattern, or is this more of a theoretical scenario? I ask because the approach seems closer to Appium's Page Object pattern than idiomatic Maestro.
I'm not against the feature itself — I think it has merit. I just want to make sure we're solving for a real use case. I tend to lean toward keeping things minimal and only adding capabilities when there's a clear, practical need.

[gdealmeida1885]

@omnarayan I think we're confusing things here a bit, perhaps because english is not my primary language. If that's the case I'm sorry, but I'll try to explain. :)

The usage of ${output.homeScreen.buttons.profile} on my example was a real use scenario, but the fix doesn't really touch that -- it was only a code snippet of my project where I've noticed the bug happening and used it to illustrated the bug.

What the PR fixes is the issue with runFlow / when / visible that was always getting ignored by maestro-runner, whenever I tried to use the conditional flow.

As for the POO, this page object pattern I used here is something that is documented on Maestro docs and it's how I've decided to approach POO on my project as its supported by the framework.

[omnarayan]

@gdealmeida1885 Thank you, There is conflict, if you can resolve it great else I ll do

[gdealmeida1885]

@omnarayan I've rebased this onto the latest main and fixed the changelog conflict. Should be good to merge now!

[omnarayan]

Thank you for the PR! Good fix — correct root cause, clean implementation, solid tests.