Extract an interface from TargetRegistry to allow wider testing support for IsEnabled #1490

kitchoi · 2021-01-19T18:15:45Z

This ~~is a draft~~ PR to shows implementations and API that ~~would~~ solve #1418.

Narrow goal:

Allow alternative registries to be written in order to support testing IsEnabled check on any QWidget or wx.Window

Wider goal:

Allow more flexibility in extending of the testing API

Changes:

All the get_* methods on the current TargetRegistry are extracted to an interface, AbstractTargetRegistry. Any objects that implement the interface can be a registry given to UIWrapper.
Some of the arguments in the get_* methods that used to require the type of UIWrapper._target is now changed to just an instance. While this adds requirements on the information required by a registry, UIWrapper._target is always an instance so that information is almost always available.
A DynamicTargetRegistry is added. Both the existing TargetRegistry and the new DynamicTargetRegistry implement the interface.
UITester now extends the given custom registries with more than one built-in registries. One of the those built-in registries is a DynamicTargetRegistry. On Qt, such registry supports querying for QWidget.isEnabled(). On wx, such registry supports wx.Window.IsEnabled(). With that get_default_registry is changed to get_default_registries to return a list instead of a single registry.
The specific implementation of IsEnabled for ButtonEditor is replaced by the more general registry.

Note:

As noted in Create an interface out of TargetRegistry to support different target types dynamically #1418, the change in parameter types on TargetRegistry.get_* is strictly speaking a change to the public API. However I believe these methods are only called by UIWrapper and is hardly useful otherwise, so it is unlikely to break downstream code. We should probably rename the methods to mark them as internal though.
It will be easy to add support for IsVisible after the changes in this PR (Add 'IsVisible` query class for UI Tester #1291)
Very early on there was an abstract base class in my draft implementation for the testing package... that abstract base class was killed early to defer decisions on generalizing an interface. With a concrete use case, the resulting interface here is different from that old one. I'm glad to have avoided early generalization/abstraction.

With regret, I will be closing this PR (without merging) once it is good enough to be kept for future reference. This was due to my lack of availability and I would not want to keep a PR open unless I know I can commit to follow through with it. I hope that this PR, as a record, helps solving #1418 in the future.

I will be pushing a few more changes before I close it.
Comments are very welcome any time!

…apper

…tion of the interface

…egistry

…mentations

kitchoi · 2021-01-20T10:16:13Z

As noted in #1418, the change in parameter types on TargetRegistry.get_* is strictly speaking a change to the public API. However I believe these methods are only called by UIWrapper and is hardly useful otherwise, so it is unlikely to break downstream code. We should probably rename the methods to mark them as internal though.

That renaming is done now - since the argument type change is a backward incompatible change, we might as well. If the old methods are ever depended on externally (I doubt it), it is easy to put them up again as a quick fix.

This PR is now clean to the standard as if this was aiming to be merged. Since (1) this is not a small PR, (2) I probably wouldn't have the bandwidth to follow it through, and (3) I don't want to rush it, I will close this PR later today. I will remove the branch too, but I believe they can be restored later from the PR.

Comments are very welcome any time (before or after me closing the PR).

aaronayres35

LGTM

I have a few comments / questions. I apologize for the extremely long delay in my review. @kitchoi if you do not currently have the bandwidth to go through with this PR, I am happy to talk with Rahul about best next steps to take for how we can get this pushed through. I think it will provide a very nice improvement for the UITester 😃, thank you!

traitsui/testing/tester/tests/test_registry.py

traitsui/testing/tester/tests/test_ui_wrapper.py

traitsui/testing/tester/_dynamic_target_registry.py

aaronayres35 · 2021-03-17T14:58:45Z

traitsui/testing/tester/_dynamic_target_registry.py

+)
+
+
+class DynamicTargetRegistry(AbstractTargetRegistry):


Naming things is hard. I don't necessarily dislike this name, but there are ways I can think about it that make it confusing.

For instance, the register_interaction and register_location methods are only defined on TargetRegistry but not on the abc, or this class.
In that sense, the normal TargetRegistry is dynamic as the interactions and locations it supports can change throughout the lifetime of the registry instance. This class is more fixed in that sense.

However, as you mention this DynamicTargetRegistry is "A registry to support targets with dynamic checks." so in that sense the name is appropriate.

Not really a suggested change here, just a comment 🤔

Naming things is hard indeed. I see your point, the word "dynamic" could well be used to describe 'TargetRegistry', in a different way. I couldn't think of a better name at the moment. Suggestions welcome!

aaronayres35 · 2021-03-17T15:03:11Z

traitsui/testing/tester/target_registry.py

@@ -288,7 +292,7 @@ def get_location_doc(self, target_class, locator_class):
            If the given locator and target types are not supported.
        """
        self._location_registry.get_value(
-            target_class=target_class,
+            target_class=target.__class__,
            key=locator_class,
        )
        # This maybe configurable in the future via register_location


😄
These changes all make a lot of sense to me. The only methods we expect users to call are really register_interaction and register_location, for which the api has remained unchanged.
Everything else (ie. all the get_* methods), I agree, are likely to not have been used externally, and really shouldn't be.

It may be slightly confusing (or surprising I guess?) that these other TargetRegistry methods now typically take a target itself rather than a target_class as an argument, but register_interaction and register_location still take a target_class, however I think that is perfectly okay. It makes sense that registering an interaction or location is a general act, that applies to all instances of a particular class.

aaronayres35 · 2021-03-17T15:13:41Z

traitsui/testing/tester/_dynamic_target_registry.py

+            target_class=target.__class__,
+            locator_class=locator_class,
+            supported=[],
+        )


Not for this PR, but will we want to add a section to the documentation about DynamicTargetRegsitry? This is in a privately named module so I assume you were thinking not / we don't expect users to mess with it.

As far as users wanting to extend UITester functionality, I feel like I could imagine scenarios where it would be useful to work with an additional DynamicTargetRegistry rather than a TargetRegistry to append to the default registries already provided on a UITester. For example right now, if this went in and say hypothetically we never put in an IsVisible implementation along with this. A downstream user may want to do something like that?
However, I can also see an argument for this only being used internally, and keeping the means of extending UITester functionality exactly as it is now.

I think I started out keeping this internal, but I am totally open to exposing this publicly if it is useful downstream. With this made public, then we need to think harder about the name...

aaronayres35 · 2021-03-17T15:24:47Z

traitsui/testing/tester/_dynamic_target_registry.py

+                interaction_class=interaction.__class__,
+                supported=list(self._get_interactions(target)),
+            )
+        return self.interaction_to_handler[interaction.__class__]


I understand the switch from target_class to target, but does interaction_class need to be interaction here (we only use interaction.__class__? or is that just to be consistent?
e.g. in _get_interaction_doc we use the interaction_class

I don't think there is any problem with having it be interaction. Just want to check I am not missing anything

You are right, this was done to be consistent. I should have explained this earlier. Thanks for flagging it.
(For _get_interaction_doc or get_location_doc we can't really do instances there because the documentation is associated with the type.)

kitchoi

Thank you @aaronayres35 for the review.

if you do not currently have the bandwidth to go through with this PR, I am happy to talk with Rahul about best next steps to take for how we can get this pushed through.

As much as I'd like to go through with this, I am wary of holding things up because of the lack of bandwidth. Though I do feel a responsibility here. Should I aim at addressing the review comments by the end of next week, and if I fail (not surprising, but I will aim not to), others can take over from then on?

kitchoi · 2021-03-17T15:54:49Z

traitsui/testing/tester/_dynamic_target_registry.py

+                interaction_class=interaction.__class__,
+                supported=list(self._get_interactions(target)),
+            )
+        return self.interaction_to_handler[interaction.__class__]


You are right, this was done to be consistent. I should have explained this earlier. Thanks for flagging it.
(For _get_interaction_doc or get_location_doc we can't really do instances there because the documentation is associated with the type.)

traitsui/testing/tester/_dynamic_target_registry.py

kitchoi · 2021-03-17T16:01:29Z

traitsui/testing/tester/_dynamic_target_registry.py

+)
+
+
+class DynamicTargetRegistry(AbstractTargetRegistry):


Naming things is hard indeed. I see your point, the word "dynamic" could well be used to describe 'TargetRegistry', in a different way. I couldn't think of a better name at the moment. Suggestions welcome!

kitchoi · 2021-03-17T16:02:40Z

traitsui/testing/tester/_dynamic_target_registry.py

+            target_class=target.__class__,
+            locator_class=locator_class,
+            supported=[],
+        )


I think I started out keeping this internal, but I am totally open to exposing this publicly if it is useful downstream. With this made public, then we need to think harder about the name...

traitsui/testing/tester/tests/test_registry.py

traitsui/testing/tester/tests/test_ui_wrapper.py

aaronayres35 · 2021-03-17T16:48:25Z

As much as I'd like to go through with this, I am wary of holding things up because of the lack of bandwidth. Though I do feel a responsibility here. Should I aim at addressing the review comments by the end of next week, and if I fail (not surprising, but I will aim not to), others can take over from then on?

That sounds perfectly reasonable to me. Again, if you don't have the bandwidth, don't worry about it at all. I am more than happy to take up this PR / work with Rahul to get it pushed through

…bstract class

kitchoi · 2021-03-22T22:18:06Z

@aaronayres35 I hope I have addressed all the comments. I have also updated the class docstring of DynamicTargetRegistry, please take a look. As for the name of the registry, I couldn't think of an alternative name unfortunately. With that, I'd leave the class in an internal module so that its name and relevant documentation can be better hashed out as separate PRs before exposing the class as part of the public API.

aaronayres35 · 2021-03-23T20:17:09Z

@aaronayres35 I hope I have addressed all the comments. I have also updated the class docstring of DynamicTargetRegistry, please take a look.

Still LGTM, thank you for the detailed doc strings - they are very helpful!

As for the name of the registry, I couldn't think of an alternative name unfortunately. With that, I'd leave the class in an internal module so that its name and relevant documentation can be better hashed out as separate PRs before exposing the class as part of the public API.

That makes sense, I agree

kitchoi · 2021-03-24T09:22:17Z

Thank you.

kitchoi added 13 commits January 19, 2021 12:06

Update the API of TargetRegistry. These methods are only used by UIWr…

f97ac2e

…apper

Extract interface to an abc

14c66a8

Remove unused imports

bbb4b3c

Update interface usage in UIWrapper; update test to use an implementa…

d06bbc7

…tion of the interface

Rename get_default_registry to get_default_registries

16fe173

Make get_default_registries return a list instead of a single TargetR…

1a11ddd

…egistry

Replace specific implementations of IsEnabled with more general imple…

3e8a975

…mentations

Pull out generic component into a base class for wx implementations

cba4a63

Move import so tests are skipped on the wrong toolkit

da970d9

No base class needed, a concrete class would do

27812b4

Fix docstring parameter name mismatch

da1e701

Fix out-of-date sentences in docstrings

057b8e2

Swap specific IsEnabled with more general implementation for wx

7c358df

aaronayres35 self-requested a review January 20, 2021 02:27

kitchoi added 4 commits January 20, 2021 09:18

Flake8 and copyright headers

703670d

Update comment and docstring for multiple registries

91cc8fc

Rename methods to make them protected

ce9f8a5

Add docstring about the methods being protected

dc75586

kitchoi closed this Jan 20, 2021

kitchoi deleted the 1418-target-registry-interface branch January 20, 2021 20:22

kitchoi mentioned this pull request Mar 17, 2021

Add 'IsVisible` query class for UI Tester #1291

Closed

aaronayres35 reviewed Mar 17, 2021

View reviewed changes

kitchoi commented Mar 17, 2021

View reviewed changes

kitchoi restored the 1418-target-registry-interface branch March 22, 2021 15:21

kitchoi added 4 commits March 22, 2021 15:22

Merge branch 'master' into 1418-target-registry-interface

aea0f23

Use {...} for instantiating set

2dbe925

Update docstring of DynamicTargetRegistry with adaptations from the a…

d03049c

…bstract class

Update and expand docstring for DynamicTargetRegistry

8f8024b

Rename variable to indicate new object in tests

679a299

kitchoi reopened this Mar 22, 2021

Somehow Appveyor thought the branch was stale. Retrigger.

85b27b3

kitchoi changed the title ~~Extract an interface from TargetRegistry to allow alternative registries be implemented~~ Extract an interface from TargetRegistry to allow wider support of IsEnabled Mar 22, 2021

kitchoi changed the title ~~Extract an interface from TargetRegistry to allow wider support of IsEnabled~~ Extract an interface from TargetRegistry to allow wider testing support for IsEnabled Mar 22, 2021

kitchoi marked this pull request as ready for review March 22, 2021 22:14

kitchoi requested a review from aaronayres35 March 22, 2021 22:18

aaronayres35 approved these changes Mar 23, 2021

View reviewed changes

kitchoi merged commit 4d69ae6 into master Mar 24, 2021

kitchoi deleted the 1418-target-registry-interface branch March 24, 2021 09:24

kitchoi mentioned this pull request Mar 24, 2021

Create an interface out of TargetRegistry to support different target types dynamically #1418

Closed

aaronayres35 mentioned this pull request Mar 24, 2021

Add 'IsVisible` query class for UI Tester #1552

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract an interface from TargetRegistry to allow wider testing support for IsEnabled #1490

Extract an interface from TargetRegistry to allow wider testing support for IsEnabled #1490

kitchoi commented Jan 19, 2021 •

edited by rahulporuri

kitchoi commented Jan 20, 2021 •

edited

aaronayres35 left a comment

aaronayres35 Mar 17, 2021

kitchoi Mar 17, 2021

aaronayres35 Mar 17, 2021

aaronayres35 Mar 17, 2021

kitchoi Mar 17, 2021

aaronayres35 Mar 17, 2021

kitchoi Mar 17, 2021

kitchoi left a comment

kitchoi Mar 17, 2021

kitchoi Mar 17, 2021

kitchoi Mar 17, 2021

aaronayres35 commented Mar 17, 2021

kitchoi commented Mar 22, 2021

aaronayres35 commented Mar 23, 2021

kitchoi commented Mar 24, 2021

Extract an interface from TargetRegistry to allow wider testing support for IsEnabled #1490

Extract an interface from TargetRegistry to allow wider testing support for IsEnabled #1490

Conversation

kitchoi commented Jan 19, 2021 • edited by rahulporuri

kitchoi commented Jan 20, 2021 • edited

aaronayres35 left a 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

kitchoi left a 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

aaronayres35 commented Mar 17, 2021

kitchoi commented Mar 22, 2021

aaronayres35 commented Mar 23, 2021

kitchoi commented Mar 24, 2021

kitchoi commented Jan 19, 2021 •

edited by rahulporuri

kitchoi commented Jan 20, 2021 •

edited