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 more ListStrEditor tests #869

Merged
merged 8 commits into from
Jun 8, 2020
Merged

Conversation

ievacerny
Copy link
Contributor

Adds more tests for ListStrEditor.

Also includes a minor bug fix in wx version of ListStrEditor because it was easier to fix it than to open an issue.
The bug: in _multi_selected_items_changed instead of self.value the method was trying to use self.values which doesn't exist.

I would also like to rename the test files test_liststr_editor.py -> test_liststr_adapter.py and test_liststr_editor_selection.py -> test_liststr_editor.py so that it's easier to find relevant tests. If nobody has any objections, I will do it just before merging to keep a nicer diff in the meantime.

@ievacerny ievacerny requested a review from kitchoi June 4, 2020 14:00
@ievacerny ievacerny self-assigned this Jun 4, 2020
@kitchoi
Copy link
Contributor

kitchoi commented Jun 4, 2020

If nobody has any objections, I will do it just before merging to keep a nicer diff in the meantime.

Let's do that as a separate brainless PR so that the diff is still nice when someone visits this PR in the future. :)

@ievacerny
Copy link
Contributor Author

ievacerny commented Jun 4, 2020

I tried to fix the tests via trial and error and managed to get rid of the errors on Mac, but they're still appearing on Windows. I think properly investigating the root cause of the issue (which I suspect is the same as for #854) might take a very long time -- should I just skip the failing tests all these tests on Windows?

@kitchoi
Copy link
Contributor

kitchoi commented Jun 4, 2020

Sounds reasonable to me (unless @corranwebster disagrees?).

Thank you for your efforts in trying to fix them. I know it is not easy and is very time consuming, especially with multi-platform. We definitely need to sort this out soon.

I probably have done a lot of this in the past too: skip the test and add a reason referring to an issue. This is in so far consistent with existing practice here. But among all the skipped tests, some are skipped for legitimate reasons, some are actually "conditional expected failures". In other words, I can't possibly go over 200 skipped test reasons and inspect if some of them may be passing now. As we have more of these tests, it is getter harder to track them. Really, we need a conditional "expectedFailure" decorator that does not also consider test errors as success (😠 Python!).

I will try to open a PR to add such a decorator, and see what people think about this. Please don't wait for that though as it will make backporting to the bug fix release a bit easier if it does not depend so much on new things (I assume we will backport both the fix and this PR that adds the test).

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.

Mostly LGTM. Thank you again for adding these tests that go beyond issue #791. Quite a few questions and suggestions.

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 very much. LGTM

(BTW, my earlier idea of a conditional expected failure decorator is not actually useful: the test can't even be run because running it causes test interactions.)

@ievacerny
Copy link
Contributor Author

Travis seems to be stuck. Closing and reopening to trigger it again.

@ievacerny ievacerny closed this Jun 8, 2020
@ievacerny ievacerny reopened this Jun 8, 2020
@mdickinson
Copy link
Member

Travis notifications still not working, apparently, but the build itself looks fine. I'll temporarily turn off the required status check so that this can be merged.

@ievacerny ievacerny merged commit f2139e7 into master Jun 8, 2020
@ievacerny ievacerny deleted the tst/add-more-liststr-editor-tests branch June 8, 2020 15:36
@kitchoi kitchoi added this to the Release 7.0.1 milestone Jul 7, 2020
kitchoi pushed a commit that referenced this pull request Jul 8, 2020
* Add more tests for ListStrEditor

* Minor wx ListStrEditor fix

* Add gui.process_events to fix pyface2 errors

* Rename TestListStrEditor with adapter test to TestListStrAdapter

* Add docstrings and clarification comments

* Skip tests on Windows due to potential test interactions

* Fix clear_selection usage
kitchoi added a commit that referenced this pull request Jul 8, 2020
* Fix the back button in the demo. (#799)

* Fixes for Qt DatetimeEditor (#803)

* Fixes for Qt DatetimeEditor.

* Fix handling of changes to editor bounds.

* Remove commented out code from editor example.

* And remove corresponding imports.

* Missed file with explicit defaults.

* Fixes to min/max and value out of range handling.

* Remove NameError and add default root_dir to tutor.py (#813)

* Remove NameError from tutor.py

* Take current directory as default

* Relax argument number constraint

* Rewrite nose tests to use unittest (#809)

* Rewrite nose tests to use unittest

* Rewrite remaining bare test functions

* Remove print statements from unit tests

* Replace assertEquals with assertEqual

* Fix error due to comparing None with int (#969)

* Use unittest discover instead of nose.core (#810)

* Merge pull request #829 from enthought/fix/ui-panel-alignment

Remove align centre which was causing problems

* Merge pull request #808 from enthought/fix/demo-fixes

A variety of fixes to the Demo app

* Add more tests for Enum Editor (#836)

* Add more tests for Enum Editor

* Add wx EnumEditor tests

* Add missing skipif decorator

* Add issue references to FIXMEs

* Address review comments

* Add check list editor tests (#837)

* Add check list editor tests

* Add wx tests for check list editor

* Simplify helper functions

* Add issue references to FIXMEs

* Address review comments

* Don't rely on empty list behaviour with combobox editor

* Add temporary fix to prevent wx test interactions

* Add set editor tests (#838)

* Add set editor tests

* Simplify helper functions

* Add wx tests for set editor

* Add issue reference to FIXME

* Address review comments

* Expand ordered flag tests

* Add image enum editor tests (#845)

* Add tests for image enum editor

* Docstring fixes

* Address review comments

* Skip tests that don't cleanup properly due to editor issue

* Remove unreliable image_cache checks

* Skip problematic tests on linux

* Address review comments

* Move value mapping from factory to individual editors (#848)

* Move value mapping from factory to individual editors

* Remove factory mapping references from image enum editor

* Remove unused wx helper function

* Address review comments

* Remove test FIXMEs

* Use instantiated tookit

* Merge pull request #850 from enthought/fix-demo-app-description-for-folder

Fix Demo application description if __init__.py or traits_ui_demo.jpg are not found

* BUG: Correctly implement auto-add functionality in ListStrModel. (#860)

* Move format_func, format_str and invalid traits from factory to editor (#859)

* Move format_func and format_str from factory to editor

* Move invalid from factory to editor as invalid_trait_name

* Merge pull request #871 from enthought/fix/segfault-row-reorder

FIX: Make sure new_row is positive

* Merge pull request #873 from enthought/maint-tabular-model

Maint: Normalize row index in TabularModel

* Make sure all UI in tests are disposed (#865)

* Dispose ui in test_visible_when_layout

* Wrap test with create_ui, and store_exceptions_on_all_threads

* Dispose ui and wrap it under store_exceptions... for test_ui

* Use create_ui in test_color_column

* Wrap tests with create_ui and store_exceptions_on_all_threads

* Dispose ui in test_actions

* Consistently dispose UI with create_ui in test_code_editor

* Skip two tests on wx that fail on its own

* Dispose UI in test_table_editor; these tests are failing on their own

* Remove skips; it was the developer's fault in not refreshing their devenv

* Dispose UI in test_csv_editor

* Make sure dispose must be called in test_date_editor

* Make sure dispose is called in test_date_range_editor

* Make sure dispose is called in test_datetime_editor

* Dispose ui in test_instance_editor

* Dispose ui in test_liststr_editor_selection

* Dispose ui in test_range_editor_spinner

* Dispose ui in test_range_editor_text

* Dispose ui in test_tree_editor

* Dispose UI in test_tuple_editor

* One more in test_data_frame_editor

* Remove additional store_exceptions_on_all_threads as they are orthogonal changes

* Experiment removing skips that were added due to test interactions

* Revert "Experiment removing skips that were added due to test interactions"

This reverts commit 7466c03.

* Try removing another workaround

* Revert "Try removing another workaround"

This reverts commit 090c75c.

* Revert inconsequential change to test_color_column

* Add close back just because it was there before - not sure if it is still needed.

* Add more TabularEditor tests (#874)

* Add more tests for Tabular Editor

* Minor wx TabularEditor fix

* Add brief explanation about Rows flag

* Add more ListStrEditor tests (#869)

* Add more tests for ListStrEditor

* Minor wx ListStrEditor fix

* Add gui.process_events to fix pyface2 errors

* Rename TestListStrEditor with adapter test to TestListStrAdapter

* Add docstrings and clarification comments

* Skip tests on Windows due to potential test interactions

* Fix clear_selection usage

* Fix bad TraitListEvent in ListStr and Tabular Editors (#875)

* Fix bad TraitListEvent in ListStr and Tabular Editors

* Remove test FIXMEs

* Fix unexpected format_func in RangeEditor bug (#900)

* Accept format_func, format_str and invalid parameters with RangeEditor

* Add a test to verify same behaviour

* Use kwargs in the signature

* Alternative fix to TabularAdapter crashes when column number reduces (#897)

Co-authored-by: Federico Miorelli <FedeMiorelli@users.noreply.github.com>

* Fix eerror when the columns number is reduced

* Workaround slot being called after the UI is disconnected

* destroy control (later) in dispose to prevent slots being called after dispose

* Revert "destroy control (later) in dispose to prevent slots being called after dispose"

This reverts commit 8bb6abd.

* Fix issue number in comment

* Update test name and comments

* Rework test comment

* Rewrite comment and run process_events so that bug appears independently of the workaround

* Add links from TabularEditor and TreeEditor to adapter docs (#917)

* Add links from TabularEditor and TreeEditor to adapter docs

* Fix tree node ref

* Fix Theme pickling (#915)

* Check toolkit before wx import + tests

* Address review comments

* Merge pull request #846 from enthought/maint-test-non-null-toolkit-layout-labels

TST: Add tests for layout and labels using any non-null toolkits
(cherry picked from commit 4398b3d)

Co-authored-by: Ieva <ievacerny@users.noreply.github.com>
Co-authored-by: Kit Choi <kitchoi@users.noreply.github.com>
Co-authored-by: Robert Kern <rkern@enthought.com>
Co-authored-by: Kit Yan Choi <kchoi@enthought.com>
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