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

Add a GUI test for Enum.create_editor #988

Merged
merged 5 commits into from
Apr 16, 2020
Merged

Conversation

midhun-pm
Copy link
Contributor

@midhun-pm midhun-pm commented Apr 6, 2020

Related to #965

Adds a GUI test for the create_editor method for the Enum trait.

Checklist

  • Tests
    - [ ] Update API reference (docs/source/traits_api_reference)
    - [ ] Update User manual (docs/source/traits_user_manual)
    - [ ] Update type annotation hints in traits-stubs

@midhun-pm
Copy link
Contributor Author

midhun-pm commented Apr 7, 2020

Found another issue with the Enum editor when values is a collection, that this test fails to catch as it merely iterates through all the items in the dropdrown, changes them and ensures that the value of the trait also changes. Will need to change this test, after fixing the UI issue.

UI Bug seen in the Numbers field in the image.
Scratch that, it was caused due to some inconsistency in my local environment, PR is good for review.

@midhun-pm midhun-pm changed the title Add a GUI test for Enum.create_editor [WIP] Add a GUI test for Enum.create_editor Apr 7, 2020
@midhun-pm midhun-pm changed the title [WIP] Add a GUI test for Enum.create_editor Add a GUI test for Enum.create_editor Apr 7, 2020
@midhun-pm
Copy link
Contributor Author

There is one known issue which this test case conveniently avoids. enthought/traitsui#781

traits/tests/test_enum.py Outdated Show resolved Hide resolved
@kitchoi kitchoi self-requested a review April 14, 2020 13:56
Copy link
Contributor

@kitchoi kitchoi left a comment

Choose a reason for hiding this comment

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

Thank you for looking into this and identify the traitsui issue. It is certainly worth doing this exercise.

traits/tests/test_enum.py Outdated Show resolved Hide resolved
traits/tests/test_enum.py Outdated Show resolved Hide resolved
traits/tests/test_enum.py Outdated Show resolved Hide resolved
traits/tests/test_enum.py Outdated Show resolved Hide resolved
traits/tests/test_enum.py Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Apr 14, 2020

Codecov Report

Merging #988 into master will increase coverage by 1.71%.
The diff coverage is 96.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #988      +/-   ##
==========================================
+ Coverage   73.05%   74.77%   +1.71%     
==========================================
  Files          51       51              
  Lines        6514     6457      -57     
  Branches     1309     1279      -30     
==========================================
+ Hits         4759     4828      +69     
+ Misses       1363     1258     -105     
+ Partials      392      371      -21     
Impacted Files Coverage Δ
traits/ctrait.py 71.07% <ø> (+12.51%) ⬆️
traits/has_traits.py 72.17% <ø> (+0.92%) ⬆️
traits/trait_base.py 68.83% <ø> (+5.25%) ⬆️
traits/trait_types.py 72.31% <92.59%> (+0.66%) ⬆️
traits/trait_list_object.py 97.51% <97.05%> (+23.22%) ⬆️
traits/testing/optional_dependencies.py 95.00% <100.00%> (+0.55%) ⬆️
traits/trait_handlers.py 61.11% <100.00%> (ø)
traits/editor_factories.py 79.59% <0.00%> (-9.19%) ⬇️
traits/trait_type.py 77.21% <0.00%> (-1.10%) ⬇️
traits/trait_dict_object.py 75.44% <0.00%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bac9f7b...386ede7. Read the comment docs.

@kitchoi
Copy link
Contributor

kitchoi commented Apr 16, 2020

I am not sure if this is ready for review again...
If we are aiming at test coverage by launching the GUI without getting too involved with the details, this is all we need?:

    def test_create_editor(self):
        obj = EnumCollectionGUIExample()

        # Create a UI window
        ui = obj.edit_traits()
        try:
            self.gui.process_events()
        finally:
            with self.delete_widget(ui.control):
                ui.dispose()

@midhun-pm
Copy link
Contributor Author

I am not sure if this is ready for review again...
If we are aiming at test coverage by launching the GUI without getting too involved with the details, this is all we need?:

    def test_create_editor(self):
        obj = EnumCollectionGUIExample()

        # Create a UI window
        ui = obj.edit_traits()
        try:
            self.gui.process_events()
        finally:
            with self.delete_widget(ui.control):
                ui.dispose()

Oh, I thought that we were going to keep it the way it is, but I came up with the current code after watching a lunch and learn by Corran and don't have much experience testing GUIs that use traits. Should I change it to the test you suggested ?

@kitchoi
Copy link
Contributor

kitchoi commented Apr 16, 2020

The situation is a bit unique here for traits, because supposedly traits should not depend on traitsui, so we'd want a more lightweight tests. For applications using traits and traitsui, it would make sense to have some integration tests that exercises GUI logic.

@mdickinson
Copy link
Member

@kitchoi Yes, I think that's all we need; just enough to exercise the code in the create_editor methods, and no more. Any of the details of the UI should be tested in TraitsUI, not Traits.

Copy link
Contributor

@kitchoi kitchoi left a comment

Choose a reason for hiding this comment

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

LGTM!

@midhun-pm midhun-pm merged commit 2685c7a into master Apr 16, 2020
@midhun-pm midhun-pm deleted the test/enum_create_editor branch April 16, 2020 14:44
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.

4 participants