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

Implement test input methods for desktop #260

Merged
merged 2 commits into from
Jul 26, 2022
Merged

Conversation

igordmn
Copy link
Collaborator

@igordmn igordmn commented Jul 21, 2022

Fixes JetBrains/compose-multiplatform#1216
Fixes JetBrains/compose-multiplatform#1846
Fixes JetBrains/compose-multiplatform#2059
Fixes JetBrains/compose-multiplatform#1127

Partially fixes JetBrains/compose-multiplatform#1177

API changes:

  • make PointerButton non-experimental
  • add a new non-experimental constructor for ComposeScene
  • TestPointerInputEventData is deprecated

It is needed, because we can only use non-experimental API from ui-test module.

During upstream, if new public API will be changed, we have to maintain its backward compatibility.

Fixes JetBrains/compose-multiplatform#1216
Fixes JetBrains/compose-multiplatform#1846
Fixes JetBrains/compose-multiplatform#2059
Fixes JetBrains/compose-multiplatform#1127

Partially fixes JetBrains/compose-multiplatform#1177

API changes:
- make PointerButton non-experimental
- add a new non-experimental constructor for ComposeScene
- TestPointerInputEventData is deprecated

During upstream, if new public API will be changed, we have to maintain its backward compatability.
@igordmn igordmn requested a review from eymar July 21, 2022 23:16
Copy link
Collaborator

@eymar eymar left a comment

Choose a reason for hiding this comment

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

Looks great! Left some questions to learn more.

@@ -504,6 +533,9 @@ class ComposeScene internal constructor(
// - move from one point of the window to another (owner == lastMoveOwner): Move
// - move from one popup to another (owner != lastMoveOwner): [Popup 1] Exit, [Popup 2] Enter

// TODO(demin) How it should behave for touch?
// We need to decide whether we need Enter/Exit events
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to understand why this question even arise:
My understanding is that Move events with Touch always imply that the pointer state isPressed=true and it means that lastMoveOwner doesn't change even if a pointer goes out of owner's bounds. Is it right?

It seems to be the same as when I pressDown the mouseButton on an item, and then move the mouse out of the item bounds to another item while holding the mouse button pressed. The lastMoveOwner won't change in this case - it will be the same first item, right? And the second item (where I moved the mouse to) won't receive Enter event.

If the above is correct, then I think there is no need to do anything extra for Touch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My understanding is that Move events with Touch always imply that the pointer state isPressed=true and it means that lastMoveOwner doesn't change even if a pointer goes out of owner's bounds. Is it right?

Yes, we still receive move events if the mouse/touch is pressed.

But this is not about keeping the owner on the pressed state. It is about "hover" events Enter/Exit, which were added additionally after we already had Move events. I am not sure how we should emit them in case of Touch. With mouse everything is simple - they are emitted when we hover the component. With touch we can't hover the component until we touch the screen.

Also, native Android behaves strange when we use Mouse - Enter/Exit emit correctly, but when we touch the screen, mouse disappears, and we receive an Exit event.

Copy link
Collaborator

@eymar eymar Jul 22, 2022

Choose a reason for hiding this comment

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

With touch we can't hover the component until we touch the screen.

yes.

Do we need enter/exit events only for Hover? Are there other use cases?


This is not about this PR. Just some thoughts:

Let's consider a scenario:

There is a Box. Using Touch, I press on it and hold it pressed and Move the pointer within its bounds.
Should it receive an Enter event? It feels like it should not. Maybe because I think about Enter event as when a pointer is initially outside the box and then moves towards it and crosses the bounds of the box.

Another scenario:
There are Box1 and Box2 (without space between them). Using Touch, I press on Box1 and move the pointer into Box2. I think Box2 won't receive any event at all, since all events will be dispatched to Box1. Should Box1 receive Exit event when pointer moved out of its bounds? If it didn't receive an Enter event (according to Scenario 1) , then it seems that receiving an Exit could be weird.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we need enter/exit events only for Hover? Are there other use cases?

We could also play some sound signal on enter, or show a tooltip. But that all relate to hover. Maybe we can even tell that hover and Enter/Exit are the same things :). The question, does it make sense to treat touch inside a Box as Hover (or as Enter).

event as when a pointer is initially outside the box

If the pointer is a finger (touch) then it is initially outside the box before the touch, and probably is inside during the touch.

I think about Enter event as when a pointer is initially outside the box and then moves towards it and crosses the bounds of the box.

The pointer can also be initially inside the Box on the first appear, so there is no crossing of the bounds. If we treat Enter/Exit as the change of the isPointerInsideBoxOrCaptured state, it will simplify things.

Box2 won't receive any event at all, since all events will be dispatched to Box1

We can send Enter as soon as Box1 loses the captured state (on a Release).

If it didn't receive an Enter event (according to Scenario 1) , then it seems that receiving an Exit could be weird.

Yes, the general rule is we only receive an Exit, if there was an Enter. Otherwise there will be a lot of inconsistency.

After our conversation, I start to think that we should send Enter even on touch. Probably before the Press. And send Exit after the Release. It isn't the case now, because we only send Enter on Move. And it probably means that we need to create synthetic events (there was one physical press, but we will send Enter + Press).


@OptIn(ExperimentalTestApi::class, ExperimentalComposeUiApi::class)
class ComposeUiTestTest {
private val events = mutableListOf<PointerEvent>()
Copy link

@dima-avdeev-jb dima-avdeev-jb Jul 25, 2022

Choose a reason for hiding this comment

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

In tests, you add elements to the MutableList, but you never clear it. Maybe you need to make an @After function in which to clear the MutableList?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When JUnit runs a new test, it makes a new instance of ComposeUiTestTest, so no need to clean resources after every test.

@igordmn igordmn merged commit 870e55c into jb-main Jul 26, 2022
@igordmn igordmn deleted the feature/fixMouseTest branch July 26, 2022 10:41
igordmn added a commit that referenced this pull request Aug 18, 2022
* Implement test input methods for desktop

Fixes JetBrains/compose-multiplatform#1216
Fixes JetBrains/compose-multiplatform#1846
Fixes JetBrains/compose-multiplatform#2059
Fixes JetBrains/compose-multiplatform#1127

Partially fixes JetBrains/compose-multiplatform#1177

API changes:
- make PointerButton non-experimental
- add a new non-experimental constructor for ComposeScene
- TestPointerInputEventData is deprecated

During upstream, if new public API will be changed, we have to maintain its backward compatability.

* replace performTouchInput to performMouseInput in tests
igordmn added a commit that referenced this pull request Aug 18, 2022
* Implement test input methods for desktop

Fixes JetBrains/compose-multiplatform#1216
Fixes JetBrains/compose-multiplatform#1846
Fixes JetBrains/compose-multiplatform#2059
Fixes JetBrains/compose-multiplatform#1127

Partially fixes JetBrains/compose-multiplatform#1177

API changes:
- make PointerButton non-experimental
- add a new non-experimental constructor for ComposeScene
- TestPointerInputEventData is deprecated

During upstream, if new public API will be changed, we have to maintain its backward compatability.

* replace performTouchInput to performMouseInput in tests
@JuBan1
Copy link

JuBan1 commented Sep 2, 2022

Thanks for implementing these functions!

However, as it is, performTextInput seems to be almost prohibitively slow. Testing with a very simple setup:

@Test
fun test() {
    var text by mutableStateOf("")
    rule.setContent {
        TextField(text, onValueChange = { text = it }, Modifier.testTag("tag"))
    }
    rule.onNode(hasTestTag("tag")).performTextInput("different text")

    rule.onNode(hasTestTag("tag")).assertTextContains("different text")
}

With the performTextInput it takes about ~2-4 seconds to run the test. Without the action, it takes about ~500ms.

The times are massively inconsistent but it slows things down significantly. It also seems that the first few tests suffer from this slowdown while subsequent tests generally become faster. Perhaps it's some kind of class loading / caching issues?

This is a screenshot of a test suite of a slightly more complex UI (a simple login composable), and I copy-pasted the same tests and classes several times. All of these tests are the exact same. They each contain two performTextInput

image

This is the same test suite, only with the performTextInput calls removed:

image

eymar pushed a commit that referenced this pull request Oct 26, 2022
* Implement test input methods for desktop

Fixes JetBrains/compose-multiplatform#1216
Fixes JetBrains/compose-multiplatform#1846
Fixes JetBrains/compose-multiplatform#2059
Fixes JetBrains/compose-multiplatform#1127

Partially fixes JetBrains/compose-multiplatform#1177

API changes:
- make PointerButton non-experimental
- add a new non-experimental constructor for ComposeScene
- TestPointerInputEventData is deprecated

During upstream, if new public API will be changed, we have to maintain its backward compatability.

* replace performTouchInput to performMouseInput in tests
eymar pushed a commit that referenced this pull request Nov 16, 2022
* Implement test input methods for desktop

Fixes JetBrains/compose-multiplatform#1216
Fixes JetBrains/compose-multiplatform#1846
Fixes JetBrains/compose-multiplatform#2059
Fixes JetBrains/compose-multiplatform#1127

Partially fixes JetBrains/compose-multiplatform#1177

API changes:
- make PointerButton non-experimental
- add a new non-experimental constructor for ComposeScene
- TestPointerInputEventData is deprecated

During upstream, if new public API will be changed, we have to maintain its backward compatability.

* replace performTouchInput to performMouseInput in tests
eymar pushed a commit that referenced this pull request Jan 13, 2023
* Implement test input methods for desktop

Fixes JetBrains/compose-multiplatform#1216
Fixes JetBrains/compose-multiplatform#1846
Fixes JetBrains/compose-multiplatform#2059
Fixes JetBrains/compose-multiplatform#1127

Partially fixes JetBrains/compose-multiplatform#1177

API changes:
- make PointerButton non-experimental
- add a new non-experimental constructor for ComposeScene
- TestPointerInputEventData is deprecated

During upstream, if new public API will be changed, we have to maintain its backward compatability.

* replace performTouchInput to performMouseInput in tests
igordmn added a commit that referenced this pull request Nov 15, 2023
* Implement test input methods for desktop

Fixes JetBrains/compose-multiplatform#1216
Fixes JetBrains/compose-multiplatform#1846
Fixes JetBrains/compose-multiplatform#2059
Fixes JetBrains/compose-multiplatform#1127

Partially fixes JetBrains/compose-multiplatform#1177

API changes:
- make PointerButton non-experimental
- add a new non-experimental constructor for ComposeScene
- TestPointerInputEventData is deprecated

During upstream, if new public API will be changed, we have to maintain its backward compatability.

* replace performTouchInput to performMouseInput in tests
igordmn added a commit that referenced this pull request Nov 16, 2023
* Implement test input methods for desktop

Fixes JetBrains/compose-multiplatform#1216
Fixes JetBrains/compose-multiplatform#1846
Fixes JetBrains/compose-multiplatform#2059
Fixes JetBrains/compose-multiplatform#1127

Partially fixes JetBrains/compose-multiplatform#1177

API changes:
- make PointerButton non-experimental
- add a new non-experimental constructor for ComposeScene
- TestPointerInputEventData is deprecated

During upstream, if new public API will be changed, we have to maintain its backward compatability.

* replace performTouchInput to performMouseInput in tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants