Add optional field to commands

[tokou]

Proposed changes

• Adds an optional flag to all commands without exception
• Adds a catch at the level of executeCommand in Orchestra to throw a CommandSkipped exception in case of an error
• Cleans up some compiler warnings

I'm not sure of the direction taken here and have some questions:

• Is it really pertinent to add optional to every single command? I feel like some command never fail 🤔
• Should we get rid of the optional in ElementSelector? Or is there a case where it makes sense to have it?
• I used the CommandSkipped exception but is the skipped state the same as an optional command failing? Should I create a separate state?

Testing

• I added some optional: true commands to 076_optional_assertion.yml
• ./gradlew test

Formats

I ran ./maestro-cli/build/install/maestro/bin/maestro test maestro-test/src/test/resources/076_optional_assertion.yaml

Running on emulator-5556

 ║
 ║  > Flow
 ║
 ║    ⚠️ Scrolling DOWN until id: not_found is visible. (warned)
 ║    ⚠️ Assert that false is true (warned)
 ║    ⚠️ Assert that id: not_found is visible (warned)
 ║    ⚪️ Assert that (Optional) "Button" is visible (skipped)
 ║

Issues fixed

Fixes #1778
Fixes #1969

[bartekpacia]

Hey @tokou, thanks a ton! I love the work done here!

I used the CommandSkipped exception but is the skipped state the same as an optional command failing? Should I create a separate state?

I was also thinking about it recently. I think a new state CommandWarned should be added, because we do want to differentiate between the optional runFlow and optional tapOn - the former should be "skipped", while the latter should "warn". wdyt?

[bartekpacia]

I think it's ok to add optional to all existing commands. We already have label argument on all commands.

Also, we won't spend time deciding if a specific command can fail or not.

[bartekpacia]

Restarting tests because of a flake:

Details

Also occurred in:

• Handle multiple flow files in test command #1995 (comment)
• Add installApp command #1854 (comment)

Reported as #2005

[bartekpacia]

That workflow run failure is a false positive from #2005, safe to ignore for now.

[bartekpacia]

@tokou This gets messed up with `optio

Take this flow as an example:

# This flow is to ensure that commands with optional flag are not failing the flow.
appId: com.example.example
tags:
    - android
    - passing
---
- launchApp:
    clearState: true
- assertTrue:
    condition: ${ false }
    label: Warn
    optional: true
- tapOn:
    id: non-existent-id
    optional: true
- doubleTapOn:
    id: non-existent-id
    optional: true
- longPressOn:
    id: non-existent-id
    optional: true
- copyTextFrom:
    id: non-existent-id
    optional: true
- launchApp:
    appId: non.existend.app.id
    optional: true
- tapOn:
    id: non-existent-id
    repeat: 3
    delay: 500
    optional: true
- scrollUntilVisible:
    element:
      id: non-existent-id
    optional: true

Results in:

./maestro-cli/build/install/maestro/bin/maestro test e2e/workspaces/demo_app/commands_optional_tournee.yaml

Running on emulator-5554

 ║
 ║  > Flow
 ║
 ║    ✅  Launch app "com.example.example" with clear state
 ║    ⚠️ Warn (warned)
 ║    ✅  Tap on (Optional) id: non-existent-id
 ║    ✅  Double tap on (Optional) id: non-existent-id
 ║    ✅  Long press on (Optional) id: non-existent-id
 ║    ⚠️ Copy text from element with (Optional) id: non-existent-id (warned)
 ║    ⚠️ Launch app "non.existend.app.id" (warned)
 ║    ✅  Tap x3 on (Optional) id: non-existent-id
 ║    ⚠️ Scrolling DOWN until (Optional) id: non-existent-id is visible. (warned)
 ║

[bartekpacia]

Action taken: I am getting rid of optional inside selector.

[bartekpacia]

Possible breaking change investigation

Removing optional from Selectors could have an unintended consequence of breaking flows that use optional in selectors at deep levels using childOf.

I created the following flow to test this hypothesis:

appId: com.example.example
tags:
    - android
    - passing
---
- launchApp:
    clearState: true
- tapOn: Nesting Test
- assertVisible:
    id: level-0
- assertVisible:
    id: level-0
    containsChild:
      id: level-1
      containsChild:
        id: level-2
        containsChild:
          id: level-3
          optional: true

and the following hierarchy (app source code):

level-0 \
  level-1 \
    level-2

without level-3

The above flow fails on latest Maestro v1.38.1, which is not what I expected. I expected it to pass, given that optional should work on selector level. But it doesn't.

$ maestro test --include-tags passing,failing --exclude-tags ios e2e/workspaces/demo_app/breaking_change_demo.yaml
Running on emulator-5554

 ║
 ║  > Flow
 ║
 ║    ✅  Launch app "com.example.example" with clear state
 ║    ✅  Tap on "Nesting Test"
 ║    ✅  Assert that id: level-0 is visible
 ║    ❌  Assert that id: level-0, Contains child: id: level-1, Contains child: id: level-2, Contains child: (Optional) id: level-3 is visible
 ║

Assertion is false: id: level-0, Contains child: id: level-1, Contains child: id: level-2, Contains child: (Optional) id: level-3 is visible

==== Debug output (logs & screenshots) ====

/Users/bartek/.maestro/tests/2024-09-04_172510

So the conclusion is: there will be no breaking change, because what I thought will break, doesn't work.

[bartekpacia]

Same goes for rightOf with optional: true (and presumably others). This fails, whereas I'd expect it to pass (i.e. be skipped)

appId: com.example.example
tags:
    - android
    - passing
---
- launchApp:
    clearState: true
- tapOn: Nesting Test
- assertVisible:
    id: level-0
- assertVisible:
    id: level-0
    containsChild:
      id: level-1
      containsChild:
        id: level-2
      rightOf:
        text: nothing
        optional: true