Issue 7774049: Adding single test runner in the flavor of NDB. (Closed)
DescriptionAdding single test runner in the flavor of NDB.
Patch Set 1 #
Total comments: 18
Patch Set 2 : Addressing review comments. #
Total comments: 4
Patch Set 3 : Adding windows todo and more descriptive comment. #
Total comments: 2
Patch Set 4 : Moving import location hack into main() instead of using a global. #Patch Set 5 : Adding quotes around which command in comment. #
MessagesTotal messages: 12
https://codereview.appspot.com/7774049/diff/1/endpoints_proto_datastore/endpo... File endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py (right): https://codereview.appspot.com/7774049/diff/1/endpoints_proto_datastore/endpo... endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py:16: IMPORT_LOCATION = None Can you add a comment to explain what this variable does? https://codereview.appspot.com/7774049/diff/1/endpoints_proto_datastore/endpo... endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py:17: TESTED_MODULES = ['utils'] s/TESTED_MODULES/MODULES_TO_TEST/ ? https://codereview.appspot.com/7774049/diff/1/endpoints_proto_datastore/endpo... endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py:21: """Changes import path to make all dependencies import correctly. s/dependancies/App Engine dependencies/ ? https://codereview.appspot.com/7774049/diff/1/endpoints_proto_datastore/endpo... endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py:41: if not os.path.exists(dev_appserver_on_path): What happens if dev_appserver_on_path is the empty string? So we test: if not dev_appserver_on_path: .... https://codereview.appspot.com/7774049/diff/1/endpoints_proto_datastore/endpo... endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py:42: print >>sys.stderr, ('Dev Appserver path %r does not exist' % In the case where "which dev_appserver.py" returns the empty string (i.e. on OSX) this error message won't make sense. https://codereview.appspot.com/7774049/diff/1/endpoints_proto_datastore/endpo... endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py:49: dev_appserver.fix_sys_path() add comment above this line to say what this does https://codereview.appspot.com/7774049/diff/1/endpoints_proto_datastore/endpo... endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py:52: ['git', 'rev-parse', '--show-toplevel']).strip() what happens if git is not on the path? https://codereview.appspot.com/7774049/diff/1/endpoints_proto_datastore/endpo... endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py:57: IMPORT_LOCATION = 'endpoints_proto_datastore' Seems like these three lines don't belong in a function called 'fix_up_path()'
Sign in to reply to this message.
Addressing review comments.
Sign in to reply to this message.
https://codereview.appspot.com/7774049/diff/1/endpoints_proto_datastore/endpo... File endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py (right): https://codereview.appspot.com/7774049/diff/1/endpoints_proto_datastore/endpo... endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py:16: IMPORT_LOCATION = None On 2013/03/26 17:44:54, fredsa.google wrote: > Can you add a comment to explain what this variable does? Done. https://codereview.appspot.com/7774049/diff/1/endpoints_proto_datastore/endpo... endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py:17: TESTED_MODULES = ['utils'] On 2013/03/26 17:44:54, fredsa.google wrote: > s/TESTED_MODULES/MODULES_TO_TEST/ ? Done. https://codereview.appspot.com/7774049/diff/1/endpoints_proto_datastore/endpo... endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py:21: """Changes import path to make all dependencies import correctly. It is more than just GAE, also adds the project to the path. Also, see the "Performs the following" bit directly below. On 2013/03/26 17:44:54, fredsa.google wrote: > s/dependancies/App Engine dependencies/ ? https://codereview.appspot.com/7774049/diff/1/endpoints_proto_datastore/endpo... endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py:41: if not os.path.exists(dev_appserver_on_path): Check out the updated error template string. On 2013/03/26 17:44:54, fredsa.google wrote: > What happens if dev_appserver_on_path is the empty string? So we test: > > if not dev_appserver_on_path: > .... https://codereview.appspot.com/7774049/diff/1/endpoints_proto_datastore/endpo... endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py:42: print >>sys.stderr, ('Dev Appserver path %r does not exist' % On OS X: In [1]: import subprocess In [2]: subprocess.check_output(['which', 'x']) --------------------------------------------------------------------------- CalledProcessError Traceback (most recent call last) <ipython-input-2-33d51dfd5266> in <module>() ----> 1 subprocess.check_output(['which', 'x']) /System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.pyc in check_output(*popenargs, **kwargs) 542 if cmd is None: 543 cmd = popenargs[0] --> 544 raise CalledProcessError(retcode, cmd, output=output) 545 return output 546 CalledProcessError: Command '['which', 'x']' returned non-zero exit status 1 On 2013/03/26 17:44:54, fredsa.google wrote: > In the case where "which dev_appserver.py" returns the empty string (i.e. on > OSX) this error message won't make sense. https://codereview.appspot.com/7774049/diff/1/endpoints_proto_datastore/endpo... endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py:49: dev_appserver.fix_sys_path() On 2013/03/26 17:44:54, fredsa.google wrote: > add comment above this line to say what this does Done. https://codereview.appspot.com/7774049/diff/1/endpoints_proto_datastore/endpo... endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py:52: ['git', 'rev-parse', '--show-toplevel']).strip() This will fail with non-zero exit status. I'll leave the fix for that up to the dev that successfully checks out this project using git while simultaneously not having git on their path. On 2013/03/26 17:44:54, fredsa.google wrote: > what happens if git is not on the path? https://codereview.appspot.com/7774049/diff/1/endpoints_proto_datastore/endpo... endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py:57: IMPORT_LOCATION = 'endpoints_proto_datastore' Suggestions on where? On 2013/03/26 17:44:54, fredsa.google wrote: > Seems like these three lines don't belong in a function called 'fix_up_path()'
Sign in to reply to this message.
https://codereview.appspot.com/7774049/diff/5001/endpoints_proto_datastore/en... File endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py (right): https://codereview.appspot.com/7774049/diff/5001/endpoints_proto_datastore/en... endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py:43: dev_appserver_on_path = subprocess.check_output( I think it would be useful to add a comment which indicates that an O/S error here will cause python to exit. https://codereview.appspot.com/7774049/diff/5001/endpoints_proto_datastore/en... endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py:43: dev_appserver_on_path = subprocess.check_output( Add a TODO to handle windows?
Sign in to reply to this message.
Adding windows todo and more descriptive comment.
Sign in to reply to this message.
https://codereview.appspot.com/7774049/diff/5001/endpoints_proto_datastore/en... File endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py (right): https://codereview.appspot.com/7774049/diff/5001/endpoints_proto_datastore/en... endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py:43: dev_appserver_on_path = subprocess.check_output( On 2013/03/26 21:23:55, fredsa.google wrote: > I think it would be useful to add a comment which indicates that an O/S error > here will cause python to exit. Done. https://codereview.appspot.com/7774049/diff/5001/endpoints_proto_datastore/en... endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py:43: dev_appserver_on_path = subprocess.check_output( On 2013/03/26 21:23:55, fredsa.google wrote: > I think it would be useful to add a comment which indicates that an O/S error > here will cause python to exit. Done.
Sign in to reply to this message.
LGTM - two comments left really would like the last three lines of fix_up_path() to be moved out of that function. Alternatively, perhaps rename fix_up_path() to reflect the additional work it's doing. https://codereview.appspot.com/7774049/diff/1/endpoints_proto_datastore/endpo... File endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py (right): https://codereview.appspot.com/7774049/diff/1/endpoints_proto_datastore/endpo... endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py:57: IMPORT_LOCATION = 'endpoints_proto_datastore' On 2013/03/26 18:33:41, dhermes wrote: > Suggestions on where? > > On 2013/03/26 17:44:54, fredsa.google wrote: > > Seems like these three lines don't belong in a function called 'fix_up_path()' > Why not after the call to fix_up_path() in main() ? https://codereview.appspot.com/7774049/diff/12001/endpoints_proto_datastore/e... File endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py (right): https://codereview.appspot.com/7774049/diff/12001/endpoints_proto_datastore/e... endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py:44: # is not on the path, then which will exit with status code 1 and Final suggestion here: put single quotes around 'which'
Sign in to reply to this message.
Moving import location hack into main() instead of using a global.
Sign in to reply to this message.
Adding quotes around which command in comment.
Sign in to reply to this message.
https://codereview.appspot.com/7774049/diff/1/endpoints_proto_datastore/endpo... File endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py (right): https://codereview.appspot.com/7774049/diff/1/endpoints_proto_datastore/endpo... endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py:57: IMPORT_LOCATION = 'endpoints_proto_datastore' Thanks for pressing on this. I think the final version reads much better. On 2013/03/26 22:15:50, fredsa.google wrote: > On 2013/03/26 18:33:41, dhermes wrote: > > Suggestions on where? > > > > On 2013/03/26 17:44:54, fredsa.google wrote: > > > Seems like these three lines don't belong in a function called > 'fix_up_path()' > > > > Why not after the call to fix_up_path() in main() ? https://codereview.appspot.com/7774049/diff/12001/endpoints_proto_datastore/e... File endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py (right): https://codereview.appspot.com/7774049/diff/12001/endpoints_proto_datastore/e... endpoints_proto_datastore/endpoints_proto_datastore_test_runner.py:44: # is not on the path, then which will exit with status code 1 and On 2013/03/26 22:15:50, fredsa.google wrote: > Final suggestion here: put single quotes around 'which' Done.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
