Skip to content

fix: stabilize Android E2E tests #3132

Merged
amanjeetsingh150 merged 11 commits intopr/no-ai-artifactsfrom
pr/contains-child-fix
Apr 6, 2026
Merged

fix: stabilize Android E2E tests #3132
amanjeetsingh150 merged 11 commits intopr/no-ai-artifactsfrom
pr/contains-child-fix

Conversation

@amanjeetsingh150
Copy link
Copy Markdown
Collaborator

@amanjeetsingh150 amanjeetsingh150 commented Apr 4, 2026

Summary

  • Fix containsChild selector when used with relative position.
  • Adding missing screens on flutter app
  • Give time to settle in waitForAnimationtoEnd
  • Disables ansi output.

The Bug

The current implementation of containsChild filter triggers 2 hierarchy calls one while constructing filter and one when it actually evaluates the filter.

This created situations when we are comparing two different hierarchies for evaluation, of the filter, one tree node made from call site and one that gets triggered while actual evaluation:

Hierarchy Call #1 (during buildFilter):
     → finds child_button → captures TreeNode_A

  Hierarchy Call #2 (during findElementWithTimeout polling):
     → gets fresh hierarchy → passes all nodes to filter chain
     → filter checks: does parent_layout.children contain TreeNode_A?

Since nodes can be different between hierarchy, sometimes this filter was able to catch value but other time it just failed.

If you check other filters they never do their own hierarchy call they just perform the actual filter logic on the incoming nodes.

Approach

This was actually hard to catch and was flaky my approach was simple try to use artifacts and give to claude to see if we have any theories I wrote a skill here: #3133

Once i had this reason I entered in planning mode to first reproduce in IntegrationTest, verified with a very similar test like the one on E2E and then change production.

Test Plan

  • Existing containsChild tests pass
  • New integration test (Case 139) for containsChild with relative position

Comment thread CONTAINSCHILD_FIX.md Outdated
@Fishbowler
Copy link
Copy Markdown
Contributor

Screenshot_20260404-093317_GitHub.png

Wild stuff happening here

@amanjeetsingh150 amanjeetsingh150 marked this pull request as draft April 4, 2026 08:36
@amanjeetsingh150 amanjeetsingh150 changed the base branch from main to pr/no-ai-artifacts April 4, 2026 13:59
@amanjeetsingh150
Copy link
Copy Markdown
Collaborator Author

@Fishbowler this seems like the ansi output was not able to get parsed properly on ci. I'm gonna try to disable it, anyways that was made for ci's

- passing
- android
- revisit
---
Copy link
Copy Markdown
Collaborator Author

@amanjeetsingh150 amanjeetsingh150 Apr 4, 2026

Choose a reason for hiding this comment

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

@Fishbowler I saw the comment here:

Why doesn't this stop our demo app? adb command behind killApp doesn't work either.

This is kinda expected but I do see its confusing, what you might expect is behaviour of stopApp.

killApp on maestro uses adb shell am kill which says:

Kill all processes associated with package. This command kills only processes that are safe to kill and that will not impact the user experience.

https://developer.android.com/tools/adb#am

This means the foreground is treated as not safe since it has higher priority, that means all other background process can be killed. Few points:

  • For, killApp though we might want to write a test that can help asserting this logic, this is what skill recommended:
  appId: com.example.example
  tags:
    - passing
    - android
  ---
  # Launch and increment counter
  - launchApp
  - assertVisible: "0"
  - tapOn:
      id: ".*fab.*"  # or whatever the FAB's identifier is
  - assertVisible: "1"

  # Background the app so am kill can work
  - pressKey: Home

  # Kill the app (only works on backgrounded apps)
  - killApp

  # Relaunch — after process death, counter should reset to 0
  - launchApp
  - assertVisible: "0"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think another question would be should we change the semantic naming of this command this is gonna confuse users alot

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This was introduced by a user who needed to simulate process death. I wouldn't be against this command going away, to be honest. It's not working as intended, doesn't have big uptake, and could clearly be confusing.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ohh you mean there are not many people anyways using it? What'd you suggest removing it or renaming it better clear the intent?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think I'd be in favour of removing it, given it doesn't do what it intends to do, and isn't easily testable either.

@amanjeetsingh150 amanjeetsingh150 force-pushed the pr/contains-child-fix branch 2 times, most recently from 66a04b2 to 541464e Compare April 6, 2026 06:38
@amanjeetsingh150 amanjeetsingh150 changed the title fix: containsChild with relative position fix: stabilize E2E tests Apr 6, 2026
@amanjeetsingh150 amanjeetsingh150 changed the title fix: stabilize E2E tests fix: stabilize Android E2E tests Apr 6, 2026
@amanjeetsingh150 amanjeetsingh150 marked this pull request as ready for review April 6, 2026 15:53
Comment thread e2e/demo_app/lib/orientation_screen.dart
Comment thread e2e/demo_app/android/app/src/main/AndroidManifest.xml
tags:
- passing
---
- launchApp
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Gotta keep a launchApp in if you wanna run it on Cloud

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think this should actually be fixed, openLink is a valid command to open app. We should allow the flows to start with openLink. Will raise a PR for this

@amanjeetsingh150 amanjeetsingh150 merged commit a52777a into pr/no-ai-artifacts Apr 6, 2026
9 checks passed
@amanjeetsingh150 amanjeetsingh150 deleted the pr/contains-child-fix branch April 6, 2026 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants