Fix for unexpected test code #374

Merged
merged 6 commits into from May 3, 2016

Conversation

Projects
None yet
3 participants
@stefanoborini
Contributor

stefanoborini commented May 3, 2016

Fix for a few strange things. The dictionary is changed in place, so it must be copied.
Additionally, it's probably supposed to call tearDown, not setUp()

Fix for unexpected test code
Fix for a few strange things. The dictionary is changed in place, so it must be copied.
Additionally, it's probably supposed to call tearDown, not setUp()
@kitchoi

This comment has been minimized.

Show comment
Hide comment
@kitchoi

kitchoi May 3, 2016

Member

Can you specify which tests fail, if possible?

Member

kitchoi commented May 3, 2016

Can you specify which tests fail, if possible?

@stefanoborini

This comment has been minimized.

Show comment
Hide comment
@stefanoborini

stefanoborini May 3, 2016

Contributor

This one, but it only fails on OSX for some reason.

======================================================================
ERROR: Test probe_data
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/travis/build/enthought/mayavi/mayavi/tests/test_mlab_integration.py", line 236, in tearDown
    for engine in registry.engines:
RuntimeError: dictionary changed size during iteration
Contributor

stefanoborini commented May 3, 2016

This one, but it only fails on OSX for some reason.

======================================================================
ERROR: Test probe_data
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/travis/build/enthought/mayavi/mayavi/tests/test_mlab_integration.py", line 236, in tearDown
    for engine in registry.engines:
RuntimeError: dictionary changed size during iteration
mayavi/tests/test_mlab_integration.py
else:
- for engine in registry.engines:
+ for engine in registry.engines[:]:

This comment has been minimized.

@kitchoi

kitchoi May 3, 2016

Member

You meant list(registry.engines)?

@kitchoi

kitchoi May 3, 2016

Member

You meant list(registry.engines)?

This comment has been minimized.

@stefanoborini

stefanoborini May 3, 2016

Contributor

@kitchoi it's the same, but I just realized it's a dictionary, not a list.

@stefanoborini

stefanoborini May 3, 2016

Contributor

@kitchoi it's the same, but I just realized it's a dictionary, not a list.

@kitchoi

This comment has been minimized.

Show comment
Hide comment
@kitchoi

kitchoi May 3, 2016

Member

less_than_vtk_5_6 is not what it looks! it is in fact less_than_vtk_5_10!

Member

kitchoi commented May 3, 2016

less_than_vtk_5_6 is not what it looks! it is in fact less_than_vtk_5_10!

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io May 3, 2016

Current coverage is 45.51%

Merging #374 into master will not change coverage

@@             master       #374   diff @@
==========================================
  Files           254        254          
  Lines         22885      22885          
  Methods           0          0          
  Messages          0          0          
  Branches       3027       3027          
==========================================
  Hits          10414      10414          
  Misses        11756      11756          
  Partials        715        715          

Powered by Codecov. Last updated by 503c807...d21739b

codecov-io commented May 3, 2016

Current coverage is 45.51%

Merging #374 into master will not change coverage

@@             master       #374   diff @@
==========================================
  Files           254        254          
  Lines         22885      22885          
  Methods           0          0          
  Messages          0          0          
  Branches       3027       3027          
==========================================
  Hits          10414      10414          
  Misses        11756      11756          
  Partials        715        715          

Powered by Codecov. Last updated by 503c807...d21739b

mayavi/tests/test_mlab_integration.py
@@ -211,29 +211,29 @@ def test_colorbar(self):
################################################################################
class TestMlabPipeline(TestMlabNullEngine):
""" Test the pipeline functions.
- For vtk versions greater than 5.6 (5.10.1 onwards), widgets need
+ For vtk versions greater than 5.10.1, widgets need

This comment has been minimized.

@kitchoi

kitchoi May 3, 2016

Member

Is 5.10.0 a thing?

@kitchoi

kitchoi May 3, 2016

Member

Is 5.10.0 a thing?

This comment has been minimized.

@stefanoborini

stefanoborini May 3, 2016

Contributor

I think we can safely assume that patchlevel is irrelevant (hopefully)

@stefanoborini

stefanoborini May 3, 2016

Contributor

I think we can safely assume that patchlevel is irrelevant (hopefully)

This comment has been minimized.

@kitchoi

kitchoi May 3, 2016

Member

I agree. So perhaps 5.10 onwards is more true to the implementation in the code.

@kitchoi

kitchoi May 3, 2016

Member

I agree. So perhaps 5.10 onwards is more true to the implementation in the code.

mayavi/tests/test_mlab_integration.py
if ver.vtk_major_version >= 5 and ver.vtk_minor_version >= 10:
- self.less_than_vtk_5_6 = False
- if self.less_than_vtk_5_6:
+ self.less_than_or_equal_to_vtk_5_10 = False

This comment has been minimized.

@kitchoi

kitchoi May 3, 2016

Member

less_than_vtk_5_10 since ver.vtk_minor_version >= 10

@kitchoi

kitchoi May 3, 2016

Member

less_than_vtk_5_10 since ver.vtk_minor_version >= 10

mayavi/tests/test_mlab_integration.py
else:
- for engine in registry.engines:
+ for engine in registry.engines.keys():

This comment has been minimized.

@kitchoi

kitchoi May 3, 2016

Member

In Python3, keys would be a view, so I would go for list(registry.engines) or list(registry.engines.keys().

@kitchoi

kitchoi May 3, 2016

Member

In Python3, keys would be a view, so I would go for list(registry.engines) or list(registry.engines.keys().

This comment has been minimized.

@stefanoborini

stefanoborini May 3, 2016

Contributor

Good point.

@stefanoborini

stefanoborini May 3, 2016

Contributor

Good point.

stefanoborini added some commits May 3, 2016

@stefanoborini

This comment has been minimized.

Show comment
Hide comment
@stefanoborini

stefanoborini May 3, 2016

Contributor

Failure seems unrelated, restarting.

Contributor

stefanoborini commented May 3, 2016

Failure seems unrelated, restarting.

@kitchoi

This comment has been minimized.

Show comment
Hide comment
@kitchoi

kitchoi May 3, 2016

Member

Assuming tests would pass with Linux + 5.10 (as it does with Mac), it looks good to me

Member

kitchoi commented May 3, 2016

Assuming tests would pass with Linux + 5.10 (as it does with Mac), it looks good to me

@stefanoborini

This comment has been minimized.

Show comment
Hide comment
@stefanoborini

stefanoborini May 3, 2016

Contributor

Tested on 5.10 ubuntu, seems to work. Merging.

Contributor

stefanoborini commented May 3, 2016

Tested on 5.10 ubuntu, seems to work. Merging.

@stefanoborini stefanoborini merged commit 4ad6afa into master May 3, 2016

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@stefanoborini stefanoborini deleted the feature/fix-for-unexpected-test-code-0503-1349 branch May 3, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment