feat: add `device.tap()` and `device.longPress()`. #4542

gayablau · 2024-08-01T07:24:17Z

Description

This pull request fixes Introduce device.tap(point), device.longPress(point) API's #4331

In this pull request, I have added api for device tap and long press for android and IOS
fixed review comments in #4534

For features/enhancements:

I have added/updated the relevant references in the documentation files.

For API changes:

I have made the necessary changes in the types index file.

…eat/device-tap

…eat/device-tap-ios

…eat/device-tap

…e-tap

…eat/device-tap-ios

asafkorem · 2024-09-14T15:35:55Z

Unit test failed due to code coverage issues.
@gayablau please make sure your tests cover the following lines (or remove them if they are dead-codes).

File	% Stmts	% Branch	% Funcs	% Lines	Uncovered Line #s
invocationTraceDescriptions.js	98.07	93.33	98.03	98.07	6
mapDeviceLongPressArguments.js	91.66	94.44	100	91.66	34-36

asafkorem

Relatively small comments compared to this massive, impressive, awesome work. This is REALLY great work, @gayablau. 100 / 100! 🫡 We are close!

asafkorem · 2024-09-14T15:42:50Z

detox/android/detox/src/full/java/com/wix/detox/espresso/EspressoDetox.java

+
+            @Override
+            public void perform(UiController uiController, View view) {
+                int adjustedY = shouldIgnoreStatusBar ? y + UiAutomatorHelper.getStatusBarHeight(view) : y;


let's extract that to private method that will be shared with the longPress's perform (reuse & separation of concerns) → e.g. calculateAdjustedY(y, shouldIgnoreStatusBar) or something similar.

asafkorem · 2024-09-14T15:53:57Z

detox/android/detox/src/full/java/com/wix/detox/espresso/UiAutomatorHelper.java

+    @SuppressLint({"DiscouragedApi", "InternalInsetResource"})
+    public static int getStatusBarHeight(View view) {
+        Context context = view.getContext();
+        int resourceId = context.getResources().getIdentifier("status_bar_height", "dimen", "android");
+        return (int) (context.getResources().getDimensionPixelSize(resourceId) / ((float) context.getResources().getDisplayMetrics().densityDpi / DisplayMetrics.DENSITY_DEFAULT));
+    }


Just to avoid confusion, could you please exctrat the method with the same name from isDisplayingAtLeastDetoxMatcher to this file? They perform a similar task but return coordinates in different units (px vs dp), so I would suggest creating two distinct method names to differentiate them:

getStatusBarHeightPixels

getStatusBarHeightDps

Detox/detox/android/detox/src/full/java/com/wix/detox/espresso/matcher/IsDisplayingAtLeastDetoxMatcher.kt

Lines 140 to 144 in 8fc7c6e

@SuppressLint("InternalInsetResource", "DiscouragedApi")

private fun getStatusBarHeight(view: View): Int {

val resourceId = view.context.resources.getIdentifier("status_bar_height", "dimen", "android")

return if (resourceId > 0) view.context.resources.getDimensionPixelSize(resourceId) else 0

}

just a scout rule that'll make the code cleaner 🙏

I changed the names, do you mean to move one of the functions?

asafkorem · 2024-09-14T16:11:51Z

detox/ios/DetoxXCUITestRunner/DetoxXCUITestRunner/Handlers/ActionHandler.swift

+    func findElement(from params: InvocationParams, predicateHandler: PredicateHandler) -> XCUIElement {
+
+        let element = predicateHandler.findElement(using: params)

+      let exists = element.waitForExistence(timeout: .defaultTimeout)
+      DTXAssert(
+        exists,
+        "Action failed, element with matcher `\(params.matcherDescription)` does not exist"
+      )
+        return element;
+    }


Please reindent

asafkorem · 2024-09-14T16:18:29Z

detox/ios/DetoxXCUITestRunner/DetoxXCUITestRunner/Handlers/ActionHandler.swift

+            let screenFrame = try XCUIApplication.appUnderTest().frame
+            let normalizedX = CGFloat(x) / screenFrame.width
+            let normalizedY = CGFloat(y) / screenFrame.height
+            let normalizedPoint = CGVector(dx: normalizedX, dy: normalizedY)
+            let coordinate = try XCUIApplication.appUnderTest().coordinate(withNormalizedOffset: normalizedPoint)


let's avoid repeated calls to XCUIApplication.appUnderTest() by storing it in a variable

asafkorem · 2024-09-14T16:23:27Z

detox/ios/DetoxXCUITestRunner/DetoxXCUITestRunner/Handlers/ActionHandler.swift

+        } catch  {
+            throw Error.failedToTapDeviceByCoordinates
+        }


you're going to throw this error anyway if you catch anything (Error.failedToTapDeviceByCoordinates) since you already wrapped this method with try-catch on the switch-case (on handle)

You may avoid the try-catch here and just let it be caught by the caller, or remove the duplicated try-catch from the caller(s).

I'm not even sure if we need any of these catches since they might hide other errors that are more specific. Please revisit the error throwing logic.

asafkorem · 2024-09-14T16:26:49Z

detox/ios/DetoxXCUITestRunner/DetoxXCUITestRunner/Handlers/ActionHandler.swift

+    func getNormalizedCoordinate(from params: InvocationParams) throws -> XCUICoordinate {
+        do {
+            guard let x = Int(params.params?.first ?? "100"), let y = Int(params.params?[1] ?? "100") else {
+                throw Error.missingTypeTextParam


you're throwing this error but catching it and hiding it (with failedToTapDeviceByCoordinates, see below).

also this is not the right error to throw, you might want to make this error more generic by introducing params to it, e.g. missingParam(paramName, paramType)

I removed the error because we want to allow this case, I just need the fallback

asafkorem · 2024-09-14T16:48:32Z

docs/api/device.md

+### `device.tap(point, shouldIgnoreStatusBar)`
+
+Perform a tap at arbitrary coordinates on the device's screen.
+
+#### tap parameters
+
+| parameter | platform | description | default value
+|  --------  |  -------  |  -------  | -------  |
+| point | Android & IOS | Coordinates in the element's coordinate space | `{x: 100, y: 100}`
+| shouldIgnoreStatusBar | Android | Coordinates will be measured starting from under the status bar. | `true`
+
+#### tap examples


This is going to be nitpicky and that's an nice structure, but we usually describe the parameters within bullet points and then just put examples without the headlines. For example: https://wix.github.io/Detox/docs/api/actions#swipedirection-speed-normalizedoffset-normalizedstartingpointx-normalizedstartingpointy

gayablau added 6 commits July 28, 2024 15:24

add tap android and basic test

a208592

update tests

2114892

remove only from test

cc494b9

add long press api

cb93597

android api improvements

66a7ecc

wip ios tap

760960b

gayablau changed the base branch from master to feat/device-tap August 1, 2024 07:24

gayablau and others added 21 commits August 1, 2024 10:26

Merge branch 'master' into feat/device-tap

f5f6267

improve decleration

a6058c7

Merge branch 'feat/device-tap' of https://github.com/wix/Detox into f…

c5aec9b

…eat/device-tap

Merge branch 'feat/device-tap' of https://github.com/wix/Detox into f…

5c1dc73

…eat/device-tap-ios

clean

b7c7252

wip

6cb90ea

some cleanup

ae6b2b9

map arguments'

aa7974d

Merge branch 'master' into feat/device-tap

3895b6d

wip

37fdfbd

fix tap

9eaf52f

clean code

166c680

add prop shouldIgnoreStatusBar

4082fcd

fix test

a8173e3

Merge branch 'feat/device-tap' of https://github.com/wix/Detox into f…

083b836

…eat/device-tap

Merge branch 'master' of https://github.com/wix/Detox into feat/devic…

c4c214c

…e-tap

Merge branch 'feat/device-tap' of https://github.com/wix/Detox into f…

e708f58

…eat/device-tap-ios

fix for ios

fb1ebc0

long press ios

6210ec0

clean and add tests

e45373b

expand tests

6ded4cb

gayablau changed the title ~~Feat device.tap() ios~~ feat: add device.tap() and device.longPress(). Sep 7, 2024

gayablau mentioned this pull request Sep 7, 2024

feat(android): add device.tap() and device.longPress(). #4534

Closed

2 tasks

gayablau changed the base branch from feat/device-tap to master September 7, 2024 08:42

gayablau added 2 commits September 7, 2024 12:22

documentation

1d8f092

clean

adbed53

gayablau marked this pull request as ready for review September 7, 2024 09:31

gayablau requested review from asafkorem, d4vidi and noomorph as code owners September 7, 2024 09:31

gayablau added 2 commits September 7, 2024 12:48

clean

cf4c84e

fix unit tests

d82c1ee

gayablau requested a review from gosha212 September 8, 2024 06:19

asafkorem self-assigned this Sep 11, 2024

asafkorem requested changes Sep 14, 2024

View reviewed changes

asafkorem removed request for noomorph, d4vidi and gosha212 September 14, 2024 16:59

gayablau and others added 2 commits November 6, 2024 10:56

Merge branch 'master' into feat/device-tap-ios

38c4e58

device tap and long press review

5f6c1c4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `device.tap()` and `device.longPress()`. #4542

feat: add `device.tap()` and `device.longPress()`. #4542

gayablau commented Aug 1, 2024 •

edited by d4vidi

Loading

asafkorem commented Sep 14, 2024

asafkorem left a comment •

edited

Loading

asafkorem Sep 14, 2024

asafkorem Sep 14, 2024

asafkorem Sep 14, 2024

gayablau Nov 6, 2024

asafkorem Sep 14, 2024

asafkorem Sep 14, 2024

asafkorem Sep 14, 2024

asafkorem Sep 14, 2024

asafkorem Sep 14, 2024

asafkorem Sep 14, 2024

asafkorem Sep 14, 2024

gayablau Nov 6, 2024

asafkorem Sep 14, 2024

gayablau Nov 6, 2024

	@SuppressLint("InternalInsetResource", "DiscouragedApi")
	private fun getStatusBarHeight(view: View): Int {
	val resourceId = view.context.resources.getIdentifier("status_bar_height", "dimen", "android")
	return if (resourceId > 0) view.context.resources.getDimensionPixelSize(resourceId) else 0
	}

feat: add device.tap() and device.longPress(). #4542

Are you sure you want to change the base?

feat: add device.tap() and device.longPress(). #4542

Conversation

gayablau commented Aug 1, 2024 • edited by d4vidi Loading

Description

asafkorem commented Sep 14, 2024

asafkorem left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: add `device.tap()` and `device.longPress()`. #4542

feat: add `device.tap()` and `device.longPress()`. #4542

gayablau commented Aug 1, 2024 •

edited by d4vidi

Loading

asafkorem left a comment •

edited

Loading