Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add cropOn screenshots capability #2086

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

TheKohan
Copy link

@TheKohan TheKohan commented Oct 9, 2024

Proposed changes

This PR extends current capabilities of takeScreenshot command with cropOn prop which is of an ElementSelector type.

Motivation

Visual regression testing is something that has been on the horizon for some time #1222. This PR is aiming to fill the gaps and prepare the ground for further development in this area (will fit #2078 great). Component level screenshots are something that will greatly decrease screenshot size and make component-level tests possible.

Example usage

test-screenshot.yml

appId: com.example.app
---
- launchApp
- tapOn: Playground
- takeScreenshot:
    path: 'Playground'
 ║                                                             
 ║  > Flow: test-flow                                          
 ║                                                             
 ║    ✅   Launch app "com.exapmle.com"                                                        
 ║    ✅   Tap on "Playground"                         
 ║    ✅   Take screenshot Playground                          
 ║            

test-component-screenshot.yml

appId: com.example.app
---
- launchApp
- tapOn: Playground
- takeScreenshot: 
      path: "Markdown"
      cropOn:
        id: MarkdownInput_Example
 ║                                                             
 ║  > Flow: test-flow                                          
 ║                                                             
 ║    ✅   Launch app "com.exapmle.com"      
 ║    ✅   Tap on "Playground"
 ║    ✅   Take screenshot Markdown, cropped id: MarkdownInput_Example
Playground Markdown
image image

Testing

  • would gladly accept any ideas of how to test it with confidence

@Matheeusb
Copy link
Contributor

Matheeusb commented Oct 9, 2024

Hey @TheKohan -- About the tests, you can add a E2E test in the "e2e" module. About this feature, great delivery!

@Fishbowler
Copy link
Contributor

Is there a reason to limit it to id than reuse the existing complex element selector logic that exists for other commands?

@TheKohan
Copy link
Author

Is there a reason to limit it to id than reuse the existing complex element selector logic that exists for other commands?

To be completely honest i didn't think about it, but i think you're right. Let me try to reimplement this part.

@TheKohan TheKohan changed the title feat: add partial screenshots capability feat: add cropOn screenshots capability Oct 16, 2024
@TheKohan
Copy link
Author

@Fishbowler - adjusted API acording to your suggestions, it's indeed way better this way. Thanks for the hint !

@TheKohan
Copy link
Author

Added test cases.

@TheKohan TheKohan force-pushed the feat/take-partial-screenshots branch from f436fe0 to 59ee970 Compare October 16, 2024 10:48
@TheKohan
Copy link
Author

@amanjeetsingh150 - can you review this PR ?

maestro.takeScreenshot(file.sink(), false)
}else{
maestro.takePartialScreenshot(sink = file.sink(), bounds = cropped.element.bounds, compressed = false)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
}
if (cropped == null){
maestro.takeScreenshot(file.sink(), false)
} else {
maestro.takePartialScreenshot(sink = file.sink(), bounds = cropped.element.bounds, compressed = false)
}

nit: White spaces.

@amanjeetsingh150
Copy link
Collaborator

Hey @TheKohan this LGTM, we will be trying to get this in 1.40.0 CLI version.

@amanjeetsingh150 amanjeetsingh150 added the v1.40.0 Release 1.40.0 label Nov 8, 2024
@amanjeetsingh150
Copy link
Collaborator

Can you also raise a PR to update docs here:

https://github.com/mobile-dev-inc/maestro-docs

@TheKohan
Copy link
Author

Thanks for answering, i'll try to find some time next week to cover things you've mentioned !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.40.0 Release 1.40.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants