Use unittest as test runner instead of nose by aaronayres35 · Pull Request #475 · enthought/enable

aaronayres35 · 2020-12-07T22:32:10Z

fixes #426

This PR simply updates edmtool to run using unittest instead of nose, and changes test file names to ensure they get discovered.

aaronayres35 · 2020-12-08T02:23:06Z

CI is failing with:

======================================================================
ERROR: testURLFallback (enable.savage.svg.tests.test_document.TestBrushFromColourValue)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/.edm/envs/enable-test-3.6-null-pillow/lib/python3.6/site-packages/enable/savage/svg/tests/test_document.py", line 69, in testURLFallback
    self.document.getBrushFromState().color,
  File "/home/travis/.edm/envs/enable-test-3.6-null-pillow/lib/python3.6/site-packages/enable/savage/svg/document.py", line 880, in getBrushFromState
    element = self.dereference(url)
  File "/home/travis/.edm/envs/enable-test-3.6-null-pillow/lib/python3.6/site-packages/enable/savage/svg/document.py", line 344, in dereference
    element = resources.open_svg(uri)
  File "/home/travis/.edm/envs/enable-test-3.6-null-pillow/lib/python3.6/site-packages/enable/savage/svg/document.py", line 191, in open_svg
    f = open(path)
  File "/home/travis/.edm/envs/enable-test-3.6-null-pillow/lib/python3.6/site-packages/enable/savage/svg/document.py", line 183, in <lambda>
    return path, lambda uri: BytesIO(urllib.request.urlopen(uri).read())
AttributeError: module 'urllib' has no attribute 'request'
----------------------------------------------------------------------

…urllib

kitchoi · 2020-12-08T09:22:22Z

~~This PR is going to create some trivial merge conflicts with #455.~~ Merging #455 is going to create some trivial merge conflicts for this PR. Is it okay if we merge #455 first?

aaronayres35 · 2020-12-08T13:13:22Z

~~This PR is going to create some trivial merge conflicts with #455.~~ Merging #455 is going to create some trivial merge conflicts for this PR. Is it okay if we merge #455 first?

Yep, go right ahead!

kitchoi · 2020-12-08T14:13:35Z

Yep, go right ahead!

Thank you. Done.

aaronayres35 · 2020-12-08T17:08:05Z

+    def given(_):
+        def func(_):
+            pass
+        return func
+    def sampled_from(_):
+        pass


this was a bit awkward, but because given and sampled_from are used in decorators, so even if I try to skip those tests when hypothesis_available == False the tests still fail when trying to load them with something like:

====================================================================== ERROR: kiva.tests.test_graphics_context (unittest.loader._FailedTest) ---------------------------------------------------------------------- ImportError: Failed to import test module: kiva.tests.test_graphics_context Traceback (most recent call last): File "/Users/aayres/.edm/envs/enable-test-3.6-null-pillow/lib/python3.6/unittest/loader.py", line 428, in _find_test_path module = self._get_module_from_name(name) File "/Users/aayres/.edm/envs/enable-test-3.6-null-pillow/lib/python3.6/unittest/loader.py", line 369, in _get_module_from_name __import__(name) File "/Users/aayres/Desktop/enable/kiva/tests/test_graphics_context.py", line 27, in <module> class TestAlphaBlackImage(unittest.TestCase): File "/Users/aayres/Desktop/enable/kiva/tests/test_graphics_context.py", line 33, in TestAlphaBlackImage @given(sampled_from([1.0, 0.0, 0.5])) NameError: name 'given' is not defined

There is probably a cleaner more standard way to do this sort of thing? I am not sure

hypothesis is a test dependency. I am not sure we need to skip these. We should make sure the test dependency is included in the setup.py though. CI does have hypothesis installed and these tests are run there.

To elaborate on why I am not sure we need to skip tests based on missing test dependencies: TraitsUI has packaging as its test dependency, and it was specified in extras_require. Whenever I do pip install traitsui and try to run the test suite, I am greeted with an import error due to missing packaging: The tests requiring packaging are not skipped because packaging is not available. That import error reminds me to reinstall with pip install traitsui[test]. If the test suite was permissive about missing test dependencies, then I risk forgetting to install those dependencies and end up running much fewer tests.

Test dependencies are still separated from optional dependencies due to optional features of the tested system (e.g. ArrayEditor is an optional feature of TraitsUI that depends on NumPy). When a package has optional features with optional dependencies, it is expected to be able to run the tests in the absence of those dependencies.

jwiggins · 2020-12-08T18:34:50Z



 # FIXME:
 #   These tests are broken, and Peter promised to fix it at some point.


Might be better to just delete these tests? Peter Wang hasn't worked on this code for a long time. I'm nearly certain that he has no interest in fixing these tests.

I noticed that only two of the four tests actually fail. It could also be a platform dependent behaviour, though.

FTR, these are the two tests that fail:

====================================================================== FAIL: test_bgr24_format (kiva.agg.tests.test_save.Test_Save) ---------------------------------------------------------------------- Traceback (most recent call last): File "/Users/kchoi/.edm/envs/enable-test-3.6-pyqt5-pillow/lib/python3.6/site-packages/kiva/agg/tests/test_save.py", line 23, in test_bgr24_format self.do_check_format('bgr24') File "/Users/kchoi/.edm/envs/enable-test-3.6-pyqt5-pillow/lib/python3.6/site-packages/kiva/agg/tests/test_save.py", line 43, in do_check_format self.format_output_map[fmt]) AssertionError: Lists differ: [255, 255, 255, 255, 255, 255, 255, 0, 0, 255, 0, 0] != [255, 255, 255, 255, 255, 255, 0, 0, 255, 0, 0, 255] First differing element 6: 255 0 - [255, 255, 255, 255, 255, 255, 255, 0, 0, 255, 0, 0] ? ------ + [255, 255, 255, 255, 255, 255, 0, 0, 255, 0, 0, 255] ? ++++++ ====================================================================== FAIL: test_bgra32_format (kiva.agg.tests.test_save.Test_Save) ---------------------------------------------------------------------- Traceback (most recent call last): File "/Users/kchoi/.edm/envs/enable-test-3.6-pyqt5-pillow/lib/python3.6/site-packages/kiva/agg/tests/test_save.py", line 29, in test_bgra32_format self.do_check_format('bgra32') File "/Users/kchoi/.edm/envs/enable-test-3.6-pyqt5-pillow/lib/python3.6/site-packages/kiva/agg/tests/test_save.py", line 43, in do_check_format self.format_output_map[fmt]) AssertionError: Lists differ: [255, 255, 255, 255, 255, 255, 255, 255, 255, 0, 0, 255, 255, 0, 0, 255] != [255, 255, 255, 255, 255, 255, 255, 255, 0, 0, 255, 255, 0, 0, 255, 255] First differing element 8: 255 0 - [255, 255, 255, 255, 255, 255, 255, 255, 255, 0, 0, 255, 255, 0, 0, 255] ? ------ ------ + [255, 255, 255, 255, 255, 255, 255, 255, 0, 0, 255, 255, 0, 0, 255, 255] ? ++++++ ++++++

kitchoi

Mostly LGTM. Some minor questions / suggestions. I have not compared the number of tests run before and after this change though.

kitchoi · 2020-12-08T18:22:20Z

+    etsdevtools_available = False

+
+@unittest.skipIf(not etsdevtools_available, "test requires etsdevtools")


I couldn't find the skip message in the log. I believe that's because test discovery does not use the test_suite and test functions below, and none of the methods in this test case has a name with the prefix test_: they start with the prefix check_ instead. Not sure if we still need to keep this check_ prefix. Could we remove the test_suite, test and the __main__ block below, and rename the method to test_... please? Such change would be within the scope of this PR because this PR changes how tests are discovered. The tests will still be skipped, though.

Should I also add etsdevtools as a test dependency like what was done for hypothesis?

Er... etsdevtools is a worse dependency here. Let's not go there, and kill this test in a separate PR. This PR should aim at minimal changes in the number of tests being run: the purpose is to switch test runner. It is not meaningful to keep this test given it has not been exercised, but that's orthogonal.

opened #483

kitchoi · 2020-12-08T18:28:39Z

+    def given(_):
+        def func(_):
+            pass
+        return func
+    def sampled_from(_):
+        pass


To elaborate on why I am not sure we need to skip tests based on missing test dependencies: TraitsUI has packaging as its test dependency, and it was specified in extras_require. Whenever I do pip install traitsui and try to run the test suite, I am greeted with an import error due to missing packaging: The tests requiring packaging are not skipped because packaging is not available. That import error reminds me to reinstall with pip install traitsui[test]. If the test suite was permissive about missing test dependencies, then I risk forgetting to install those dependencies and end up running much fewer tests.

Test dependencies are still separated from optional dependencies due to optional features of the tested system (e.g. ArrayEditor is an optional feature of TraitsUI that depends on NumPy). When a package has optional features with optional dependencies, it is expected to be able to run the tests in the absence of those dependencies.

…_extras_require__ under 'test'

…d remove unneeded functions

codecov-io · 2020-12-08T19:56:51Z

Codecov Report

Merging #475 (e34aeca) into master (b09f48e) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #475      +/-   ##
==========================================
- Coverage   30.45%   30.43%   -0.03%     
==========================================
  Files         206      206              
  Lines       17852    17852              
  Branches     2461     2461              
==========================================
- Hits         5437     5433       -4     
- Misses      12072    12076       +4     
  Partials      343      343

Impacted Files	Coverage Δ
enable/__init__.py	`75.00% <ø> (ø)`
enable/savage/svg/document.py	`26.80% <100.00%> (ø)`
kiva/image.py	`85.71% <0.00%> (-14.29%)`	⬇️
kiva/fonttools/font_manager.py	`48.50% <0.00%> (-2.09%)`	⬇️
enable/qt4/base_window.py	`51.69% <0.00%> (+0.61%)`	⬆️
enable/abstract_window.py	`54.19% <0.00%> (+0.76%)`	⬆️
enable/qt4/scrollbar.py	`78.73% <0.00%> (+1.14%)`	⬆️
enable/container.py	`56.85% <0.00%> (+1.67%)`	⬆️

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 b09f48e...e34aeca. Read the comment docs.

kitchoi

Thank you.
(One minor suggestion in https://github.com/enthought/enable/pull/475/files#r539141323, as I forgot to submit it as a part of a batch review)

aaronayres35 added 2 commits December 7, 2020 16:18

use unittest as test runner

49c5bec

rename test files for unittest discovery

56f9244

attempt to fix broken ci by importing urllib.request instead of just …

1a042c6

…urllib

aaronayres35 added 6 commits December 8, 2020 09:47

Merge branch 'master' into use-unittest-testrunner

2a4d8ae

another test rename

387e3ee

remove a bunch of uses of nose in tests

1c2e261

replace nose with unittest in test_component_editor

ad1a541

remove enose from list of dependencies in edm tool

0224855

add reason to skip tests

b57266d

aaronayres35 changed the title ~~Use unittest as test runner instead of nose~~ [WIP] Use unittest as test runner instead of nose Dec 8, 2020

skip tests if dependency isn't available

b0697c4

aaronayres35 commented Dec 8, 2020

View reviewed changes

missed a test with nose, replaced it with unittest

a375d33

aaronayres35 changed the title ~~[WIP] Use unittest as test runner instead of nose~~ Use unittest as test runner instead of nose Dec 8, 2020

aaronayres35 requested review from jwiggins and kitchoi December 8, 2020 17:51

jwiggins reviewed Dec 8, 2020

View reviewed changes

kitchoi reviewed Dec 8, 2020

View reviewed changes

aaronayres35 added 3 commits December 8, 2020 13:29

dont skip tests that need hypothesis, but instead add hypothesis to _…

bd7e0aa

…_extras_require__ under 'test'

flake8

a561139

apply suggestion from code review to rename methods check_->test_, an…

6099222

…d remove unneeded functions

use self.assertEqual instead of assert

e34aeca

aaronayres35 mentioned this pull request Dec 8, 2020

kiva/agg/tests/test_save tests are currently broken #480

Open

reference issue for broken tests which are skipped

61427a0

kitchoi approved these changes Dec 9, 2020

View reviewed changes

aaronayres35 mentioned this pull request Dec 9, 2020

Kill test with etsdevtools dependency #483

Closed

aaronayres35 merged commit 6b41a42 into master Dec 9, 2020

jwiggins deleted the use-unittest-testrunner branch December 9, 2020 14:26

rahulporuri mentioned this pull request Dec 31, 2020

Rename test module so it gets discovered by unittest #528

Merged



		# FIXME:
		# These tests are broken, and Peter promised to fix it at some point.

		etsdevtools_available = False


		@unittest.skipIf(not etsdevtools_available, "test requires etsdevtools")

Conversation

aaronayres35 commented Dec 7, 2020

Uh oh!

aaronayres35 commented Dec 8, 2020

Uh oh!

kitchoi commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaronayres35 commented Dec 8, 2020

Uh oh!

kitchoi commented Dec 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kitchoi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-io commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kitchoi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kitchoi commented Dec 8, 2020 •

edited

Loading

kitchoi left a comment •

edited

Loading

codecov-io commented Dec 8, 2020 •

edited

Loading