Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14969007:
Create a self test for skimage. (Closed)
|
Created:
7 years, 7 months ago by scroggo Modified:
7 years, 7 months ago Reviewers:
epoger CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionCreate a self test for skimage.
Runs skimage twice: once to create an expectations file, and a
second time comparing against the file. Uses the files in
resources as test files.
R=epoger@google.com
Committed: https://code.google.com/p/skia/source/detail?r=9123
Patch Set 1 #
Total comments: 19
Patch Set 2 : Respond to comments (but not all of them). #Patch Set 3 : #
Total comments: 6
Patch Set 4 : Respond to comments. #Patch Set 5 : Use os.pardir to represent the parent directory. #
Total comments: 4
Patch Set 6 : Respond to comments #
Messages
Total messages: 9 (0 generated)
https://codereview.chromium.org/14969007/diff/1/tools/tests/skimage/self_test.py File tools/tests/skimage/self_test.py (right): https://codereview.chromium.org/14969007/diff/1/tools/tests/skimage/self_test... tools/tests/skimage/self_test.py:1: #!/usr/bin/env python I noticed that gm's self test has a note that it should be written in python, so this is written in python. https://codereview.chromium.org/14969007/diff/1/tools/tests/skimage/self_test... tools/tests/skimage/self_test.py:23: gcc_path = "out/Debug/skimage" It looks like gm's self test assumes you have already built gm. I've followed that convention, although I ran into a funny situation on my linux machine where I had already built skimage, but not since the program had been updated. I was tempted to attempt to build, but it occurred to me that building might require different commands on different machines, and some may be set up for ninja/some other build process.
https://codereview.chromium.org/14969007/diff/1/tools/tests/skimage/self_test.py File tools/tests/skimage/self_test.py (right): https://codereview.chromium.org/14969007/diff/1/tools/tests/skimage/self_test... tools/tests/skimage/self_test.py:41: subprocess.check_call(create_expectations_cmd) There are some commands for calling programs more safely in the buildbot directory. Should those be available for me to use here?
Thanks for adding this, definitely will be helpful. Suggestions below... https://codereview.chromium.org/14969007/diff/1/tools/tests/skimage/self_test.py File tools/tests/skimage/self_test.py (right): https://codereview.chromium.org/14969007/diff/1/tools/tests/skimage/self_test... tools/tests/skimage/self_test.py:1: #!/usr/bin/env python On 2013/05/09 15:50:43, scroggo wrote: > I noticed that gm's self test has a note that it should be written in python, so > this is written in python. Yes, good idea to do it that way in the first place. :-) Ideally, I think it would be good to (eventually) call this test from https://code.google.com/p/skia/source/browse/trunk/tools/tests/run.sh , so that developers can easily run self-tests for *everything* in the tools directory. https://codereview.chromium.org/14969007/diff/1/tools/tests/skimage/self_test... tools/tests/skimage/self_test.py:11: # Use the directory of this file as the out directory I think it would be better to write test files into a separate directory, so that any given directory includes EITHER svn-controlled source code OR disposable test files, but not BOTH. Like how https://code.google.com/p/skia/source/browse/trunk/tools/tests/run.sh writes into trunk/tools/tests/skdiff and trunk/tools/tests/benchgraphs https://codereview.chromium.org/14969007/diff/1/tools/tests/skimage/self_test... tools/tests/skimage/self_test.py:23: gcc_path = "out/Debug/skimage" On 2013/05/09 15:50:43, scroggo wrote: > It looks like gm's self test assumes you have already built gm. I've followed > that convention, although I ran into a funny situation on my linux machine where > I had already built skimage, but not since the program had been updated. > > I was tempted to attempt to build, but it occurred to me that building might > require different commands on different machines, and some may be set up for > ninja/some other build process. Yeah, it's a tough call, because individual developers (or buildbots) may want to build the code in different ways. On the other hand, I have seen people get confused because they made a code change and the self-test results did not change, because they forgot to rebuild the code. I think the way you have it is good, at least for now. https://codereview.chromium.org/14969007/diff/1/tools/tests/skimage/self_test... tools/tests/skimage/self_test.py:25: if (os.path.exists(gcc_path)): Good idea to go looking for the binary in different places on different platforms! Here's another approach which would be more flexible, and would allow for either Debug or Release builds... # Ordered list of paths where we look for the skimage binary. # Paths that are earlier in the list are preferred. POSSIBLE_BINARY_PATHS = [ 'xcodebuild/Debug/skimage', 'xcodebuild/Release/skimage', 'out/Debug/skimage', 'out/Release/skimage', ] skimage_binary = PickFirstFileThatExists(POSSIBLE_BINARY_PATHS) ... and PickFirstFileThatExists() iterates over the list, returning the first one that refers to an existing file. A downside of this would be: if the user had built both Debug and Release versions, he wouldn't know which one was run. https://codereview.chromium.org/14969007/diff/1/tools/tests/skimage/self_test... tools/tests/skimage/self_test.py:38: cmd_line.extend(["-r", "resources/CMYK.jpg", "resources/plane.png"]) Alternatively, you could assemble these paths as absolute paths, and thus eliminate the need for chdir() https://codereview.chromium.org/14969007/diff/1/tools/tests/skimage/self_test... tools/tests/skimage/self_test.py:41: subprocess.check_call(create_expectations_cmd) On 2013/05/09 15:57:29, scroggo wrote: > There are some commands for calling programs more safely in the buildbot > directory. Should those be available for me to use here? Unfortunately, no. Most developers will have checked out trunk/ but not buildbot/ . The buildbots definitely check out *both*, though, so if there are scripts we want to be able to use from either trunk/ or buildbot/, we should consider moving them into trunk/ . https://codereview.chromium.org/14969007/diff/1/tools/tests/skimage/self_test... tools/tests/skimage/self_test.py:44: check_expectations_cmd = cmd_line + ["--readExpectationsPath", resultsFile] Great! This is definitely a good start... but, if I understand correctly, this proves that *any given revision* of skimage reads and writes consistent expectations. It does NOT prove that the results are the same as they were before any local changes. Please consider adding tests that will exercise that, in the future, if it's not terribly hard to do. It can be tricky, because the typical way to do that is to check expected results into SVN, and then you have to worry about results that differ by platform, rebaselining the expected results when appropriate, etc. https://codereview.chromium.org/14969007/diff/1/tools/tests/skimage/self_test... tools/tests/skimage/self_test.py:49: print "Self tests failed!" It would be better if the process exited with a nonzero return value when the self-tests fail, for uses like this: run_self_test_1 && run_self_test_2 && run_self_test_3 && echo "it all worked!"
https://codereview.chromium.org/14969007/diff/1/tools/tests/skimage/self_test.py File tools/tests/skimage/self_test.py (right): https://codereview.chromium.org/14969007/diff/1/tools/tests/skimage/self_test... tools/tests/skimage/self_test.py:11: # Use the directory of this file as the out directory On 2013/05/09 16:35:50, epoger wrote: > I think it would be better to write test files into a separate directory, so > that any given directory includes EITHER svn-controlled source code OR > disposable test files, but not BOTH. Like how > https://code.google.com/p/skia/source/browse/trunk/tools/tests/run.sh writes > into trunk/tools/tests/skdiff and trunk/tools/tests/benchgraphs Done. https://codereview.chromium.org/14969007/diff/1/tools/tests/skimage/self_test... tools/tests/skimage/self_test.py:25: if (os.path.exists(gcc_path)): On 2013/05/09 16:35:50, epoger wrote: > Good idea to go looking for the binary in different places on different > platforms! > > Here's another approach which would be more flexible, and would allow for either > Debug or Release builds... > > # Ordered list of paths where we look for the skimage binary. > # Paths that are earlier in the list are preferred. > POSSIBLE_BINARY_PATHS = [ > 'xcodebuild/Debug/skimage', > 'xcodebuild/Release/skimage', > 'out/Debug/skimage', > 'out/Release/skimage', > ] > > skimage_binary = PickFirstFileThatExists(POSSIBLE_BINARY_PATHS) > > ... and PickFirstFileThatExists() iterates over the list, returning the first > one that refers to an existing file. > > A downside of this would be: if the user had built both Debug and Release > versions, he wouldn't know which one was run. I have added a function to find the binary. I also print out which binary is used to alleviate any confusion. https://codereview.chromium.org/14969007/diff/1/tools/tests/skimage/self_test... tools/tests/skimage/self_test.py:38: cmd_line.extend(["-r", "resources/CMYK.jpg", "resources/plane.png"]) On 2013/05/09 16:35:50, epoger wrote: > Alternatively, you could assemble these paths as absolute paths, and thus > eliminate the need for chdir() Done. https://codereview.chromium.org/14969007/diff/1/tools/tests/skimage/self_test... tools/tests/skimage/self_test.py:41: subprocess.check_call(create_expectations_cmd) On 2013/05/09 16:35:50, epoger wrote: > On 2013/05/09 15:57:29, scroggo wrote: > > There are some commands for calling programs more safely in the buildbot > > directory. Should those be available for me to use here? > > Unfortunately, no. Most developers will have checked out trunk/ but not > buildbot/ . > > The buildbots definitely check out *both*, though, so if there are scripts we > want to be able to use from either trunk/ or buildbot/, we should consider > moving them into trunk/ . That sounds reasonable to me. Probably outside of the scope of this CL, but where within trunk/ would they live? Should there be a scripts directory in trunk? https://codereview.chromium.org/14969007/diff/1/tools/tests/skimage/self_test... tools/tests/skimage/self_test.py:44: check_expectations_cmd = cmd_line + ["--readExpectationsPath", resultsFile] On 2013/05/09 16:35:50, epoger wrote: > Great! This is definitely a good start... but, if I understand correctly, this > proves that *any given revision* of skimage reads and writes consistent > expectations. It does NOT prove that the results are the same as they were > before any local changes. > > Please consider adding tests that will exercise that, in the future, if it's not > terribly hard to do. It can be tricky, because the typical way to do that is to > check expected results into SVN, and then you have to worry about results that > differ by platform, rebaselining the expected results when appropriate, etc. Definitely the long term plan is to compare against previous revisions, but that will be trickier. Is there a way to merge gm_expectations files from different platforms or will I need to keep separate versions? https://codereview.chromium.org/14969007/diff/1/tools/tests/skimage/self_test... tools/tests/skimage/self_test.py:49: print "Self tests failed!" On 2013/05/09 16:35:50, epoger wrote: > It would be better if the process exited with a nonzero return value when the > self-tests fail, for uses like this: > > run_self_test_1 && run_self_test_2 && run_self_test_3 && echo "it all worked!" Done.
LGTM with or without the minor suggestions below. https://codereview.chromium.org/14969007/diff/1/tools/tests/skimage/self_test.py File tools/tests/skimage/self_test.py (right): https://codereview.chromium.org/14969007/diff/1/tools/tests/skimage/self_test... tools/tests/skimage/self_test.py:41: subprocess.check_call(create_expectations_cmd) On 2013/05/09 19:05:59, scroggo wrote: > That sounds reasonable to me. Probably outside of the scope of this CL, but > where within trunk/ would they live? Should there be a scripts directory in > trunk? Definitely outside the scope of this CL. Please coordinate with Ravi and/or Eric as far as the directory goes. https://codereview.chromium.org/14969007/diff/1/tools/tests/skimage/self_test... tools/tests/skimage/self_test.py:44: check_expectations_cmd = cmd_line + ["--readExpectationsPath", resultsFile] > Is there a way to merge gm_expectations files from different > platforms or will I need to keep separate versions? To some extent, we may be able to have expectations that work across platforms. The bitmap hash values in GM self-test work cross-platform, for example. But for encoders/decoders that yield different pixel values on different platforms, I guess we'll need separate baselines. Blah. Yes, it's for another day. :-) https://codereview.chromium.org/14969007/diff/7001/tools/tests/skimage_self_t... File tools/tests/skimage_self_test.py (right): https://codereview.chromium.org/14969007/diff/7001/tools/tests/skimage_self_t... tools/tests/skimage_self_test.py:25: return None This could raise an Exception, thus halting execution with an error status. If you give the Exception a description, it'll even be logged for you... https://codereview.chromium.org/14969007/diff/7001/tools/tests/skimage_self_t... tools/tests/skimage_self_test.py:30: file_dir = os.path.abspath(file_dir) how about just: file_dir = os.path.abspath(os.path.dirname(__file__)) https://codereview.chromium.org/14969007/diff/7001/tools/tests/skimage_self_t... tools/tests/skimage_self_test.py:56: except subprocess.CalledProcessError as e: Do you need to catch CalledProcessError? What if you just allowed the Exception to get thrown all the way to the calling shell?
https://codereview.chromium.org/14969007/diff/7001/tools/tests/skimage_self_t... File tools/tests/skimage_self_test.py (right): https://codereview.chromium.org/14969007/diff/7001/tools/tests/skimage_self_t... tools/tests/skimage_self_test.py:25: return None On 2013/05/09 19:19:39, epoger wrote: > This could raise an Exception, thus halting execution with an error status. If > you give the Exception a description, it'll even be logged for you... Done. https://codereview.chromium.org/14969007/diff/7001/tools/tests/skimage_self_t... tools/tests/skimage_self_test.py:30: file_dir = os.path.abspath(file_dir) On 2013/05/09 19:19:39, epoger wrote: > how about just: > > file_dir = os.path.abspath(os.path.dirname(__file__)) Done. https://codereview.chromium.org/14969007/diff/7001/tools/tests/skimage_self_t... tools/tests/skimage_self_test.py:56: except subprocess.CalledProcessError as e: On 2013/05/09 19:19:39, epoger wrote: > Do you need to catch CalledProcessError? What if you just allowed the Exception > to get thrown all the way to the calling shell? I do not. I caught the error to give a cleaner error message, but cleaner also means less information, so I've updated the code to all the exception to get thrown to the shell.
LGTM https://codereview.chromium.org/14969007/diff/15001/tools/tests/skimage_self_... File tools/tests/skimage_self_test.py (right): https://codereview.chromium.org/14969007/diff/15001/tools/tests/skimage_self_... tools/tests/skimage_self_test.py:14: return "Could not find binary!\nDid you forget to build the tools project?\n" \ or, I think this will also appease the Python interpreter: return ("Part 1 of string\n" "Part 2 of string") https://codereview.chromium.org/14969007/diff/15001/tools/tests/skimage_self_... tools/tests/skimage_self_test.py:42: # Run skimage twice, first to create an expectations file, and then IMHO, this would be easier to read with a newline before each comment line.
https://codereview.chromium.org/14969007/diff/15001/tools/tests/skimage_self_... File tools/tests/skimage_self_test.py (right): https://codereview.chromium.org/14969007/diff/15001/tools/tests/skimage_self_... tools/tests/skimage_self_test.py:14: return "Could not find binary!\nDid you forget to build the tools project?\n" \ On 2013/05/14 15:24:45, epoger wrote: > or, I think this will also appease the Python interpreter: > > return ("Part 1 of string\n" > "Part 2 of string") Done. https://codereview.chromium.org/14969007/diff/15001/tools/tests/skimage_self_... tools/tests/skimage_self_test.py:42: # Run skimage twice, first to create an expectations file, and then On 2013/05/14 15:24:45, epoger wrote: > IMHO, this would be easier to read with a newline before each comment line. Done.
Message was sent while issue was closed.
Committed patchset #6 manually as r9123 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
