Skip to content

Conversation

bmalrat
Copy link
Collaborator

@bmalrat bmalrat commented Aug 2, 2024

Description

Added UI Toolkits test for the Input Action Editor. https://jira.unity3d.com/browse/ISX-2087
It tests list, creation, rename and delete.

Changes made

Fixed Action Maps name edition which could be inconsistent in Input Action Editor UI. The time used used was not the editor one and the UI was destroyed and recreated on each change.

Testing

local test on windows + CI

Risk

limited to Input Action Editor UI.

Checklist

Before review:

  • Changelog entry added.
    • Explains the change in Changed, Fixed, Added sections.
    • For API change contains an example snippet and/or migration example.
    • JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
    • Jira port for the next release set as "Resolved".
  • Tests added/changed, if applicable.
    • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
    • Performance tests.
    • Integration tests.
  • Docs for new/changed API's.
    • Xmldoc cross references are set correctly.
    • Added explanation how the API works.
    • Usage code examples added.
    • The manual is updated, if needed.

During merge:

  • Commit message for squash-merge is prefixed with one of the list:
    • NEW: ___.
    • FIX: ___.
    • DOCS: ___.
    • CHANGE: ___.
    • RELEASE: 1.1.0-preview.3.

After merge:

  • Create forward/backward port if needed. If you are blocked from creating a forward port now please add a task to ISX-1444.

@bmalrat bmalrat requested a review from jfreire-unity August 5, 2024 09:10
@lyndon-unity lyndon-unity self-requested a review August 5, 2024 09:11
Copy link
Collaborator

@lyndon-unity lyndon-unity left a comment

Choose a reason for hiding this comment

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

Overall looks like a reasonable test.
Small naming query added, but happy to approve.

static readonly Vector2 k_MinWindowSize = new Vector2(650, 450);

// For UI testing purpose
internal InputActionAsset currentAssetInEdition => m_AssetObjectForEditing;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this name a typo?
currentAssetInEdition

Should it be ...
currentAssetInEditior

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed the name, thanks

Copy link
Collaborator

@ekcoh ekcoh left a comment

Choose a reason for hiding this comment

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

Awesome to see some UI automation happening for tests, good job. Some minor comments.


#region Helper methods

void Click(VisualElement ve)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome to see this being added but I wonder whether it would make sense to move these helper methods into a separate utility class to make it more accessible across any test classes that would utilise this kind of "UI automation" actions? Would also be great with a short comment on what each one does and maybe some preconditions asserts to check that the passed element is applicable to avoid mistakes?

Also would suggest renaming "ve" to "target" since it represents the target of the automated action to be carried out?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Encapsulating it might further reduce visibility of some of these methods where SendXXX is likely internal while a utility class for testing might contain "actions" like, Click, EnterText etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks I will make some name adjustment.
Most of the methods are copied from https://internaldocs.unity.com/editor_and_runtime_development_guide/TestAutomation/TestFrameworks/UITestFramework/CodeSamples/code-samples/ and will be replaced by the helpers on neutron

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the ve => target is unfortunately not as simple. For clicking it may work because it took the positon and if the target it on top the click will be dispatched to it.
For all other command even if you call visualelement.sendevent it will be dispatched to the panel which will search for te focused element.
I make some change to make it clearer

ve.SendEvent(pue);
}

void SendText(VisualElement ve, string text, bool sendReturn = true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similarly, think it could be good with some docs on these utility functions, is sendReturn used to configure whether the pretended user is hitting Return/Enter key after entering the text?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes the sendReturn is used to define if we send return at the end of the text

//a bit hacky - e.StopImmediatePropagation() for events does not work like expected on ListViewItems or TreeViewItems because
//the listView/treeView reclaims the focus - this is a workaround with less overhead than rewriting the events
DelayCall();
schedule.Execute(() => renameTextfield.Q<TextField>().Focus()).StartingIn(120);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for getting rid of that unreliable DelayCall

Copy link
Collaborator

Choose a reason for hiding this comment

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

Guess its still suffers from not being fully deterministic though...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes this delay could create some instabilities, it was hard to find somethings reliable 🤞

@bmalrat bmalrat force-pushed the isx-2087-add-ui-test branch from 1acdc0a to c4c92de Compare August 9, 2024 12:31
@bmalrat bmalrat merged commit 5556836 into develop Aug 9, 2024
@bmalrat bmalrat deleted the isx-2087-add-ui-test branch August 9, 2024 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants