Fix bugs #416

Merged
merged 10 commits into from Aug 1, 2016

Conversation

Projects
None yet
4 participants
@prabhuramachandran
Member

prabhuramachandran commented Jul 26, 2016

Fixes a silly error, issues #397, #387, add tests for visual, fix problem with picker ui not showing, and fix a hidden execfile.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jul 26, 2016

Current coverage is 50.21% (diff: 93.18%)

Merging #416 into master will increase coverage by 4.60%

@@             master       #416   diff @@
==========================================
  Files           256        257      +1   
  Lines         23292      23221     -71   
  Methods           0          0           
  Messages          0          0           
  Branches       3074       3071      -3   
==========================================
+ Hits          10623      11660   +1037   
+ Misses        11948      10816   -1132   
- Partials        721        745     +24   

Powered by Codecov. Last update 39397fb...7a41c9d

codecov-io commented Jul 26, 2016

Current coverage is 50.21% (diff: 93.18%)

Merging #416 into master will increase coverage by 4.60%

@@             master       #416   diff @@
==========================================
  Files           256        257      +1   
  Lines         23292      23221     -71   
  Methods           0          0           
  Messages          0          0           
  Branches       3074       3071      -3   
==========================================
+ Hits          10623      11660   +1037   
+ Misses        11948      10816   -1132   
- Partials        721        745     +24   

Powered by Codecov. Last update 39397fb...7a41c9d

@@ -683,6 +684,10 @@ def save_gl2ps(self, file_name, exp=None):
self._exporter_write(ex)
else:
ex.write()
+ # Work around for a bug in VTK where it saves the file as a
+ # .pdf.gz when the file is really a PDF file.

This comment has been minimized.

@mdickinson

mdickinson Jul 27, 2016

Member

Is this bug known to the VTK developers? If so, it would be useful to have a reference to a VTK issue, so that future readers of this code have a way to determine whether the upstream bug has been fixed or not.

@mdickinson

mdickinson Jul 27, 2016

Member

Is this bug known to the VTK developers? If so, it would be useful to have a reference to a VTK issue, so that future readers of this code have a way to determine whether the upstream bug has been fixed or not.

This comment has been minimized.

@prabhuramachandran

prabhuramachandran Jul 27, 2016

Member

I don't know and its hard for me to find the time to follow up but I will try to do that.

@prabhuramachandran

prabhuramachandran Jul 27, 2016

Member

I don't know and its hard for me to find the time to follow up but I will try to do that.

This comment has been minimized.

@prabhuramachandran

prabhuramachandran Jul 27, 2016

Member

I looked at the sources and it seems the GL2PS code has changed considerably with VTK 7.0 The GL2PS exporter has been completely changed and looks like the bug shouldn't be there -- but I did not test as I do not have a VTK 7 build with GL2PS enabled on my system.

@prabhuramachandran

prabhuramachandran Jul 27, 2016

Member

I looked at the sources and it seems the GL2PS code has changed considerably with VTK 7.0 The GL2PS exporter has been completely changed and looks like the bug shouldn't be there -- but I did not test as I do not have a VTK 7 build with GL2PS enabled on my system.

tvtk/pyface/ui/qt4/decorated_scene.py
@@ -158,6 +158,7 @@ def _save_snapshot(self):
"Geomview", "VRML", "Wavefront", "Open Inventor",
"Povray", "X3D"]
+ wildcard = ""

This comment has been minimized.

@mdickinson

mdickinson Jul 27, 2016

Member

Why didn't tests catch this bug? Is there an opportunity to add a unit test here?

@mdickinson

mdickinson Jul 27, 2016

Member

Why didn't tests catch this bug? Is there an opportunity to add a unit test here?

This comment has been minimized.

@prabhuramachandran

prabhuramachandran Jul 27, 2016

Member

Because it is triggered only when you click on "save" and I am not sure the best way to test that. Do you have any suggestions of doing this easily without popping up a UI? This would be super useful to have tested.

@prabhuramachandran

prabhuramachandran Jul 27, 2016

Member

Because it is triggered only when you click on "save" and I am not sure the best way to test that. Do you have any suggestions of doing this easily without popping up a UI? This would be super useful to have tested.

This comment has been minimized.

@mdickinson

mdickinson Jul 27, 2016

Member

The easiest way would probably be to use the mock package to mock out the FileDialog.open method to always return the string 'OK'.

@mdickinson

mdickinson Jul 27, 2016

Member

The easiest way would probably be to use the mock package to mock out the FileDialog.open method to always return the string 'OK'.

This comment has been minimized.

@prabhuramachandran

prabhuramachandran Jul 27, 2016

Member

Of course, but the method is private too so it smells that I have to invoke _save_snapshot but anyway, better a simple test than none. Testing UI specific stuff always leaves me with a bad taste and I am never sure if it is a good use of one's time. I wish there were easier tooling to make this easier to do or write the code in a way that we could focus on other things. For example, there are some scenarios that are painful to test, for example if you click on a traits UI color selector from within IPython with %gui=qt it disappears -- how is one supposed to be able to test this? For example try this:

mlab.test_plot3d()

Now click on the last (gear) icon on the toolbar and try changing the background color, the UI appears and disappears. How should we test this? Its frustrating that there aren't easier ways to ensure that these do not happen due to changes in traitsui/qt/pyface/whatever.

@prabhuramachandran

prabhuramachandran Jul 27, 2016

Member

Of course, but the method is private too so it smells that I have to invoke _save_snapshot but anyway, better a simple test than none. Testing UI specific stuff always leaves me with a bad taste and I am never sure if it is a good use of one's time. I wish there were easier tooling to make this easier to do or write the code in a way that we could focus on other things. For example, there are some scenarios that are painful to test, for example if you click on a traits UI color selector from within IPython with %gui=qt it disappears -- how is one supposed to be able to test this? For example try this:

mlab.test_plot3d()

Now click on the last (gear) icon on the toolbar and try changing the background color, the UI appears and disappears. How should we test this? Its frustrating that there aren't easier ways to ensure that these do not happen due to changes in traitsui/qt/pyface/whatever.

This comment has been minimized.

@mdickinson

mdickinson Jul 27, 2016

Member

I agree that accessing _save_snapshot directly would be a smell, but I don't think it's necessary: you should be able to call perform on the related Action object instead. You'd have to write logic to retrieve the relevant action in the first place from DecoratedScene.actions, and I'd suggest adding an id to the individual actions to make that easier.

@mdickinson

mdickinson Jul 27, 2016

Member

I agree that accessing _save_snapshot directly would be a smell, but I don't think it's necessary: you should be able to call perform on the related Action object instead. You'd have to write logic to retrieve the relevant action in the first place from DecoratedScene.actions, and I'd suggest adding an id to the individual actions to make that easier.

This comment has been minimized.

@mdickinson

mdickinson Jul 27, 2016

Member

And yes, I absolutely think that testing UI interactions is a good use of time: it's an essential for a professionally developed application that involves those UI interactions. There are some tools in Traits and PyFace that make testing of UI interactions somewhat easier.

@mdickinson

mdickinson Jul 27, 2016

Member

And yes, I absolutely think that testing UI interactions is a good use of time: it's an essential for a professionally developed application that involves those UI interactions. There are some tools in Traits and PyFace that make testing of UI interactions somewhat easier.

This comment has been minimized.

@prabhuramachandran

prabhuramachandran Jul 27, 2016

Member

Some pointers to documentation on these tools would be useful to have and use -- in fact it would be nice to have these documented as part of the traits manual. It has been a decade (2005?) since I read any traits/pyface documentation, so pardon my ignorance but I think the documentation on suggested patterns for this is scarce. Thanks!

@prabhuramachandran

prabhuramachandran Jul 27, 2016

Member

Some pointers to documentation on these tools would be useful to have and use -- in fact it would be nice to have these documented as part of the traits manual. It has been a decade (2005?) since I read any traits/pyface documentation, so pardon my ignorance but I think the documentation on suggested patterns for this is scarce. Thanks!

@mdickinson

This comment has been minimized.

Show comment
Hide comment
@mdickinson

mdickinson Jul 27, 2016

Member

Any chance of unit tests or regression tests for the 'visual' issue fixes?

Member

mdickinson commented Jul 27, 2016

Any chance of unit tests or regression tests for the 'visual' issue fixes?

@prabhuramachandran

This comment has been minimized.

Show comment
Hide comment
@prabhuramachandran

prabhuramachandran Jul 27, 2016

Member

@mdickinson -- Thanks, that is a good idea. visual is hardly used so has been badly neglected (as can be seen). I should invest a bit of time writing a few proper tests for it and will do so.

Member

prabhuramachandran commented Jul 27, 2016

@mdickinson -- Thanks, that is a good idea. visual is hardly used so has been badly neglected (as can be seen). I should invest a bit of time writing a few proper tests for it and will do so.

prabhuramachandran added some commits Jul 27, 2016

Fix picker point not showing.
When one presses 'p' on a point, the picked coordinate would not show
due to the use of the old pipeline methods.  This is now fixed.
Refactor the save popup code and test it.
There were four identical implementations of the `popup_save` function
that were all not tested.  This has been refactored into one with a
simple test.
@prabhuramachandran

This comment has been minimized.

Show comment
Hide comment
@prabhuramachandran

prabhuramachandran Jul 29, 2016

Member

@mdickinson -- Coverage increased by around 4.5% with this PR thanks to a bunch of tests! I also fixed a bunch of bugs. I think the PR is now safe to merge and we should cut a 4.5.0 release. Please take a look.

Member

prabhuramachandran commented Jul 29, 2016

@mdickinson -- Coverage increased by around 4.5% with this PR thanks to a bunch of tests! I also fixed a bunch of bugs. I think the PR is now safe to merge and we should cut a 4.5.0 release. Please take a look.

tvtk/tests/test_pyface_utils.py
+ m.path = 'mock'
+ return m
+
+ def test_popup_save(self):

This comment has been minimized.

@itziakos

itziakos Jul 30, 2016

Member

you could also write the symmetric test method where 'open' does not return "OK"

@itziakos

itziakos Jul 30, 2016

Member

you could also write the symmetric test method where 'open' does not return "OK"

tvtk/tests/test_visual.py
+ self.assertEqual(r.radius, 1.0)
+ self.assertEqual(r.thickness, 0.01)
+ self.assertEqual(r.tube.radius, 0.01)
+ self.assertTrue(np.allclose(r.pos, (1.0, 1.0, 1.0)))

This comment has been minimized.

@itziakos

itziakos Jul 30, 2016

Member

from numpy.testing you can use 'assert_array_almost_equal'

@itziakos

itziakos Jul 30, 2016

Member

from numpy.testing you can use 'assert_array_almost_equal'

This comment has been minimized.

@prabhuramachandran

prabhuramachandran Jul 31, 2016

Member

That one slipped, I have used it right below... Is it preferred to use assert_array_almost_equal or assert_allclose??

@prabhuramachandran

prabhuramachandran Jul 31, 2016

Member

That one slipped, I have used it right below... Is it preferred to use assert_array_almost_equal or assert_allclose??

self.property.on_trait_change(self.viewer.scene.render)
self.actor.on_trait_change(self.viewer.scene.render)
self.tube.on_trait_change(self.viewer.scene.render)
+ HasTraits.__init__(self, **traits)

This comment has been minimized.

@itziakos

itziakos Jul 30, 2016

Member

why not use "super"?

@itziakos

itziakos Jul 30, 2016

Member

why not use "super"?

This comment has been minimized.

@prabhuramachandran

prabhuramachandran Jul 31, 2016

Member

No particular reason. I wrote the tests and got it all working and fixed the issues. This can be done later if someone cares or has the time.

@prabhuramachandran

prabhuramachandran Jul 31, 2016

Member

No particular reason. I wrote the tests and got it all working and fixed the issues. This can be done later if someone cares or has the time.

Add couple of tests as per review.
Added case where user may select Cancel instead of allowing the dialog.
Also add a test case for the `visual.frame` object.
@prabhuramachandran

This comment has been minimized.

Show comment
Hide comment
@prabhuramachandran

prabhuramachandran Jul 31, 2016

Member

@itziakos -- I've fixed the issues you pointed out and added another test for the visual.frame object.

Member

prabhuramachandran commented Jul 31, 2016

@itziakos -- I've fixed the issues you pointed out and added another test for the visual.frame object.

@itziakos

This comment has been minimized.

Show comment
Hide comment
@itziakos

itziakos Jul 31, 2016

Member

@prabhuramachandran from a quick look it looks ok, especially when it improves coverage.

Member

itziakos commented Jul 31, 2016

@prabhuramachandran from a quick look it looks ok, especially when it improves coverage.

@@ -13,6 +13,7 @@
from __future__ import print_function
+import os
import os.path

This comment has been minimized.

@mdickinson

mdickinson Aug 1, 2016

Member

Doesn't import os make import os.path redundant?

@mdickinson

mdickinson Aug 1, 2016

Member

Doesn't import os make import os.path redundant?

@mdickinson mdickinson merged commit ec63af7 into enthought:master Aug 1, 2016

2 checks passed

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

@prabhuramachandran prabhuramachandran deleted the prabhuramachandran:fix-bugs branch Aug 1, 2016

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