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

RadioButtonsTests.GamepadCanEscapeAndDoesNotSelectWithFocus is unreliable. #2202

Closed
kmahone opened this issue Mar 31, 2020 · 13 comments · Fixed by #3182
Closed

RadioButtonsTests.GamepadCanEscapeAndDoesNotSelectWithFocus is unreliable. #2202

kmahone opened this issue Mar 31, 2020 · 13 comments · Fixed by #3182
Labels
area-TestInfrastructure Issue in the test infrastructure (e.g. in Helix scripts) help wanted Issue ideal for external contributors team-Controls Issue for the Controls team

Comments

@kmahone
Copy link
Member

kmahone commented Mar 31, 2020

I have disabled this test due to unreliability:
RadioButtonsTests.GamepadCanEscapeAndDoesNotSelectWithFocus

Error log:
[Error]: AreEqual(8, 9) [File d:\a\1\s\dev\RadioButtons\InteractionTests\RadioButtonsTests.cs Line: 897]

The issue is likely due to:

GamepadHelper.PressButton(null, GamepadButton.DPadDown);
        public static void PressButton(UIObject obj, GamepadButton button)
        {
            Log.Comment("Pressing Gamepad button: {0}", button);
            KeyboardInput[] array = new KeyboardInput[1];
            array[0].virtualKeyCode = (ushort)button;
            InternalNativeMethods.InjectKeyboardInput(array, 1);

            // Without a delay, the input gets dropped.
            Wait.ForMilliseconds(50);

            array[0].flags = KEY_EVENT_FLAGS.KEYUP;
            InternalNativeMethods.InjectKeyboardInput(array, 1);
        }

Likely the wait sometimes causes the button the register as being held down, resulting in repeated 'DPadDown' presses being processes, with the result that focus ends up moving too far.

@msft-github-bot msft-github-bot added the needs-triage Issue needs to be triaged by the area owners label Mar 31, 2020
@StephenLPeters StephenLPeters added area-RadioButtons team-Controls Issue for the Controls team labels Apr 1, 2020
@StephenLPeters
Copy link
Contributor

This is likely an issue for other tests as well. For instance number box's gamepad tests are failing frequently as well.

@StephenLPeters StephenLPeters added area-TestInfrastructure Issue in the test infrastructure (e.g. in Helix scripts) help wanted Issue ideal for external contributors and removed area-RadioButtons needs-triage Issue needs to be triaged by the area owners labels Apr 1, 2020
@marcelwgn
Copy link
Collaborator

I'm afraid, there isn't much we can do since we are unable to listen to any events of the objects, for example ManipulationStarted, which we could use to track if input was accepted. One thing we can do is experiment with the delay, but that won't solve the underlying reliability in a safe way.

@StephenLPeters
Copy link
Contributor

StephenLPeters commented Apr 2, 2020

We always have the test hooks tool. we could add a private test hooks api to add an event that is raised after the gamepad (or other) input has completed then the test app can bind a checkbox (or something else that we can read from the test) to that event and the test can wait for that checkbox to change state. We do something similar with swipe and teaching tip for waiting for the animations to complete. This seems really silly for this though, really we should just figure out this part:

            // Without a delay, the input gets dropped.
            Wait.ForMilliseconds(50);

@marcelwgn
Copy link
Collaborator

marcelwgn commented Apr 2, 2020

So should we then experiment with the amount we wait here?

Or try to use some internal checkbox we poll every x milliseconds?

Edit: It turns out, 5-20 milliseconds are enough on my machine for the test to work (though it did too with 50ms)

@ranjeshj
Copy link
Contributor

ranjeshj commented Apr 2, 2020

Lets get to an event model and avoid the wait for time. Test hooks sound like a reasonable solution.

@marcelwgn
Copy link
Collaborator

It seems like that we are not able to listen to key events in a fashion that would be reliable enough (or more reliable) than using a simple wait. When running tests, for some reasons KeyUp or KeyDown do not get raised.

Instead of Wait.ForMilliSeconds we could use Wait.ForIdle() since the goal of the sleep is to properly trigger the key, and then "untrigger" it. Since this is usually done to trigger some UI change, Wait.ForIdle probably would be able to detect when the key was received as some UI will change (be it focus of a control or some text).

@marcelwgn
Copy link
Collaborator

Unless someone else is looking into this, I would like to work on this.

@StephenLPeters
Copy link
Contributor

Is your plan to try replacing the wait for miliseconds with a wait for idle?

@marcelwgn
Copy link
Collaborator

It seems that in some cases, we can listen to KeyUp and have a checkbox that we could look for. In some cases however, the KeyUp get's swallowed by the control, e.g. pressing GamePadA on a Button. In that case, we either fall back to the current solution, wait for idle, or both. I am not sure what's the best solution is here.

Knowing when the key was received by the app does help us though, for example when verifying gamepad navigation as this typically does not get swallowed by the controls. So in cases where we are "moving" with the gamepad, we are able to use the KeyUp mechanic.

@StephenLPeters
Copy link
Contributor

You can register a handler that will run even for events marked as Handled using the AddHandler method.

@marcelwgn
Copy link
Collaborator

Oh I didn't knew that existed. With that, we could probably get rid of the sleep entirely.

@marcelwgn
Copy link
Collaborator

Given that #3152 was merged now, what are the next steps regarding unreliable gamepad tests?

@StephenLPeters
Copy link
Contributor

@kmahone my best idea is to enable all of them and see if they start failing. I think this ends up having a pretty high cost if the reliability isn't fixed though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-TestInfrastructure Issue in the test infrastructure (e.g. in Helix scripts) help wanted Issue ideal for external contributors team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants