Conversation
|
Looks correct to me but I didnot run the code |
chaosphere2112
left a comment
chaosphere2112
left a comment
There was a problem hiding this comment.
This test needs some work.
testing/vcs/test_script_for_py.py
Outdated
| @@ -0,0 +1,15 @@ | |||
| import vcs | |||
|
|
|||
| list=["boxfill","meshfill","isofill","textcombined","texttable","vector"] | |||
There was a problem hiding this comment.
You shouldn't name things reserved words in python (list being the type of []). It technically works but is bad form, and will break things later down the road.
There was a problem hiding this comment.
Also, you're missing a few:
"isoline", "1d", "taylordiagram", "3d_scalar", "3d_dual_scalar", "3d_vector"
Remove "textcombined" and "texttable"; these should be handled in a second loop, which will do secondary objects (marker, fillarea, colormap, line, textcombined, texttable, textorientation).
testing/vcs/test_script_for_py.py
Outdated
| vcs.createtextcombined('EXAMPLE_tt', 'qa', 'EXAMPLE_tto', '7left') | ||
| tc="'EXAMPLE_tt','EXAMPLE_tto'" | ||
| code="s=vcs.get%s(%s)" % (obj, tc) | ||
| exec(code) |
There was a problem hiding this comment.
It's bad form to use exec for... well, anything. I know it's sprinkled throughout the codebase, but you shouldn't pick up bad habits from that 😉
Instead, you can use this:
s = vcs.creategraphicsmethod(obj, 'test_%s' % obj)
which will let you generate an arbitrary graphics method.
testing/vcs/test_script_for_py.py
Outdated
| for obj in list: | ||
| s='' | ||
| tc='' | ||
| if obj == 'textcombined': |
There was a problem hiding this comment.
Let's extract the secondary graphics methods to a separate loop. Also drop the below logic.
testing/vcs/test_script_for_py.py
Outdated
| code="s=vcs.get%s(%s)" % (obj, tc) | ||
| exec(code) | ||
| try: | ||
| s.script(obj+"_script.py") |
There was a problem hiding this comment.
You need to delete the files after you generate them. Also, this only builds the script; for this to be a real test, it needs to validate correctness. That means running the scripts and seeing if the generated object matches the original.
testing/vcs/test_script_for_py.py
Outdated
| try: | ||
| s.script(obj+"_script.py") | ||
| except: | ||
| raise("Python script creation broke on "+obj+".script()") |
There was a problem hiding this comment.
This doesn't work, for a couple of reasons:
- raise is a keyword, not a function. This is just a misleadingly worded version of
raise "Python script creation broke on...". - You can only raise objects that derive from the
BaseExceptionclass in python. This usually means something like: `raise ValueError("Python script creation broke on...").
Moreover, you should just print "Python script creation broke on..." and do sys.exit(1).
|
@chaosphere2112, thank you for the review! I'll get working on the revisions. |
works, and that the object created by scriptrun() is identical to the object on which script() was called.
|
@chaosphere2112 @doutriaux1, got the test written, but it looks like I don't have push access to the uvcdat repo any more. If anyone's here Tuesday I'll get admin help and push it then. |
|
@embrown i think iot's because travis ci fails then olny admins can approve. |
|
pretty uch every test fails there... Let me take a look... |
ghost
mentioned this pull request
3 participants
Test for CDAT/vcs#104
@doutriaux1 Did I do this correctly?