Add more tests for Enum Editor

[ievacerny]

Adds more tests for EnumEditor.

[kitchoi]

Thank you for adding the tests. This is great.
I noticed that the test fails on wx due to existing issues which you have opened issues for. It is good to have these tests first so that we can fix them with TDD.

Just one change request to add an else branch for future-proofing and consistency. The rest are nitpicks / non-actionable comments / may-be-out-of-scope suggestions that should not block this PR.

[kitchoi]

Nit: It might be worth noting the assumption that all the children widgets are radio buttons.

[kitchoi]

These functions allow the tests to be run on different toolkits with the minimal necessary toolkit-specific adaptations, which is quite nice. In the code base, there exists an alternative approach: a new test is created for each toolkit and the test is skipped altogether if the toolkit does not match the expected, e.g. https://github.com/enthought/traitsui/blob/master/traitsui/tests/test_actions.py

The alternative approach has one benefit: If traitsui should support a new toolkit, this would not cause a lot of existing tests targeting (wx or Qt) to fail.

In the approach here, if the toolkit is not null (so the tests are not skipped), and if both is_current_backend_wx and is_current_backend_qt4 return false, the functions do nothing. Then the tests may either do nothing meaningful, or fail in some mysterious way.

To be consistent with the other tests, we should add an else branch in these functions to raise SkipTest. <--- The only change request here.

We could also refactor the pattern to something along this line (that could be in a separate PR so not to block this one):

def toolkit_dispatch(func):
    """ Decorator for wrapping a default implementation of a toolkit-dependent
    function. Toolkit-specific implementation can be registered to the
    returned callable using one of the toolkit supported in ETSConfig.

    Example:

    @toolkit_dispatch
    def get_text(widget):
        # this is the default implementation
        # Typically it is appropriate to raise NotImplementedError.

    @get_text.register("wx")
    def get_text(widget):
        return widget.GetName()
    """

    toolkit_to_implementation = {}

    def register(toolkit):

        def wrapped(implementation):
            toolkit_to_implementation[toolkit] = implementation
        return wrapped

    def wrapper(*args, **kwargs):
        implementation = toolkit_to_implementation.get(ETSConfig.toolkit, func)
        return implementation(*args, **kwargs)

    wrapper.register = register
    return wrapper

It will be easier to add an implementation to a mapping, instead of modifying if-else branches inline. The implementations, being independent, will also be correctly decoupled from each other.

[ievacerny]

Will add an else branch 👍 Although I don't see why tests for existing toolkits would start failing even if else branch isn't added -- am I missing something?

As for the new pattern, to me adding an elif branch to a helper method seems easier than going through this framework -- there is only one docstring and one signature to maintain and adding

elif new_toolkit:
    implementation

seems easier than

@function.register("new_toolkit")
def function(signature):
    implementation

[kitchoi]

Although I don't see why tests for existing toolkits would start failing even if else branch isn't added -- am I missing something?

Sorry for my weird way of putting things.

If the else branch isn't added, and if we have a new toolkit, when we run the test suite with this new toolkit, the tests here will not be skipped (because the toolkit isn't null). Since is_current_backend_wx and is_current_backend_qt4 will return false, the tests will likely fail because the helper functions are no-op in that new toolkit.

[kitchoi]

there is only one docstring and one signature to maintain and adding

I would probably maintain the docstring once only like this:

@toolkit_dispatch
def get_something(widget):
    """ Docstring here """

@get_something.register("wx")
def _(widget):
    return widget.GetName()

This is similar to the functools.singledispatch: It is a functional way to achieve polymorphism.

[kitchoi]

Just an observation: The create_ui context manager being introduced in https://github.com/enthought/traitsui/pull/664/files could be useful here. I will try to review that PR soon.

[ievacerny]

I'm not sure it would be that useful here because GUI for wx tests would still have to be set up and cleaned up manually.

[kitchoi]

Yes, we are missing the GUITestAssistant for Wx too: enthought/pyface#266

In any case, what I meant to say is that this is a pattern repeated so many times that it needs to be refactored. I was also meaning to reference an implementation where this use case is solved using a context manager.

[ievacerny]

Yes, I agree about refactoring but I don't think there's a need to rush the review on #664 because it wouldn't help here until the same exists for wx. And I don't think that's going to happen soon enough to make it into this PR.

[kitchoi]

LGTM!