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.

Sign up for GitHub

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

Jump to bottom

[plugin-mobile-app] Add full view shooting strategy #2568

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[plugin-mobile-app] Add full view shooting strategy #2568

wants to merge 1 commit into from

Conversation

uarlouski
Copy link
Member

@uarlouski uarlouski commented Mar 12, 2022 •
edited by valfirst
Loading

Closes #2005

@vividus-framework vividus-framework deleted a comment from lgtm-com bot Mar 15, 2022
@vividus-framework vividus-framework deleted a comment from lgtm-com bot Mar 15, 2022
@vividus-framework vividus-framework deleted a comment from lgtm-com bot Mar 18, 2022
@vividus-framework vividus-framework deleted a comment from sonarcloud bot Mar 18, 2022
@vividus-framework vividus-framework deleted a comment from lgtm-com bot Mar 18, 2022
@vividus-framework vividus-framework deleted a comment from codecov bot Mar 18, 2022
@vividus-framework vividus-framework deleted a comment from lgtm-com bot Mar 18, 2022
@codecov
Copy link

codecov bot commented Mar 21, 2022 •
edited
Loading

Codecov Report

Merging #2568 (2de8b52) into master (a48e057) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #2568      +/-   ##
============================================
+ Coverage     96.69%   96.72%   +0.02%     
- Complexity     5739     5768      +29     
============================================
  Files           811      813       +2     
  Lines         16361    16475     +114     
  Branches       1059     1064       +5     
============================================
+ Hits          15821    15935     +114     
  Misses          427      427              
  Partials        113      113
Impacted Files Coverage Δ
...s/selenium/screenshot/AbstractScreenshotTaker.java 96.15% <100.00%> (-0.28%) ⬇️
...g/vividus/selenium/screenshot/ScreenshotUtils.java 100.00% <100.00%> (ø)
...c/main/java/org/vividus/steps/ui/BarcodeSteps.java 100.00% <100.00%> (ø)
.../src/main/java/org/vividus/ui/util/ImageUtils.java 100.00% <100.00%> (ø)
...ava/org/vividus/mobileapp/action/TouchActions.java 98.46% <100.00%> (+0.10%) ⬆️
...s/selenium/mobileapp/MobileAppScreenshotTaker.java 100.00% <100.00%> (ø)
...shot/strategies/MobileAppFullShootingStrategy.java 100.00% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@uarlouski uarlouski changed the title debug [plugin-mobile-app] Add full view shooting strategy Mar 22, 2022
@uarlouski uarlouski marked this pull request as ready for review March 22, 2022 14:35
@uarlouski
Copy link
Member Author

Why do full-page screenshots have a system footer? Is this expected?

yes, this is handled in FixedCutStrategy decorator that is provided with header in footer to cut (AbstractAshotFactory)

@uarlouski uarlouski requested a review from ikalinin1 April 8, 2022 11:34
@uarlouski uarlouski requested a review from valfirst April 12, 2022 11:22
ikalinin1
ikalinin1 reviewed Apr 13, 2022
View reviewed changes
vividus-plugin-mobile-app/build.gradle
@@ -9,6 +9,7 @@ project.description = 'Vividus plugin for testing mobile applications'

implementation(group: 'org.slf4j', name: 'slf4j-api', version: versions.slf4j)
implementation(group: 'javax.inject', name: 'javax.inject', version: versions.javaxInject)
implementation(group: 'org.boofcv', name: 'boofcv-core', version: '0.40.1')
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we have it as an extension to the visual testing plugin?

Copy link
Member Author

Choose a reason for hiding this comment

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

  • If we want to make the strategy standalone then we will also have to move things out from plugin mobile like touch actions etc into separate plugin, so we will have two more modules for what?
  • 254.7 with boofcv vs 241 without boofcv doesn't seem like a big win

ikalinin1
ikalinin1 reviewed Apr 13, 2022
View reviewed changes
docs/modules/plugins/pages/plugin-visual.adoc
Comment on lines +270 to +296
==== Strategies

[cols="1,3", options="header"]
|===

|Name
|Description

|`SIMPLE`
|Used to take a screenshot of current viewport. This strategy is used by default.

|`FULL`
|Used to take a screenshot of whole application view. The strategy starts shooting at the top position
of the application view and ends at the bottom position, once the shooting is done the initial top position
gets restored.

For iOS platform make sure to set the `screenshotQuality` property to `0` to ensure screenshot pixels
consistensy, please see https://github.com/appium/appium-xcuitest-driver[XCUI capabililties] for more details.

|===

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
==== Strategies
[cols="1,3", options="header"]
|===
|Name
|Description
|`SIMPLE`
|Used to take a screenshot of current viewport. This strategy is used by default.
|`FULL`
|Used to take a screenshot of whole application view. The strategy starts shooting at the top position
of the application view and ends at the bottom position, once the shooting is done the initial top position
gets restored.
For iOS platform make sure to set the `screenshotQuality` property to `0` to ensure screenshot pixels
consistensy, please see https://github.com/appium/appium-xcuitest-driver[XCUI capabililties] for more details.
|===
==== Strategies
[cols="1,3", options="header"]
|===
|Name
|Description
|`SIMPLE`
|Used to take a screenshot of the current viewport. This strategy is used by default.
|`FULL`
|Used to take a screenshot of the whole application view. The strategy starts shooting at the top position
of the application view and ends at the bottom position, once the shooting is done the initial top position
gets restored.
For iOS platform, make sure to set the `screenshotQuality` property to `0` to ensure screenshot pixels
consistency, please see https://github.com/appium/appium-xcuitest-driver[XCUI capabilities] for more details.
|===

Copy link
Member

Choose a reason for hiding this comment

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

Will ignore area/element work for this strategy?

Copy link
Member Author

Choose a reason for hiding this comment

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

WIP

@sonarcloud
Copy link

sonarcloud bot commented Apr 27, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@sonarcloud
Copy link

sonarcloud bot commented Jun 21, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@valfirst valfirst requested a review from a team as a code owner February 18, 2023 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ikalinin1 ikalinin1 ikalinin1 left review comments

@valfirst valfirst Awaiting requested review from valfirst

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add possibility to perform visual checks for FULL screen on mobile devices
3 participants
@uarlouski @ikalinin1 @valfirst