Refactor test_pictures so we can add test_pdfs without code duplication.

Refactor test_pictures so we can add test_pdfs without code duplication. 
Committed: https://code.google.com/p/skia/source/detail?r=5878

[edisonn, 2012-10-05 15:50:44]

Refactor test_pictures so we can add test_pdfs without code duplication.

[epoger, 2012-10-05 19:54:13]

LGTM

https://codereview.appspot.com/6611050/diff/3001/tools/test_pictures.py
File tools/test_pictures.py (left):

https://codereview.appspot.com/6611050/diff/3001/tools/test_pictures.py#oldco...
tools/test_pictures.py:27: def RunCommand(command):
It's probably too late now, but it seems like the diff could have been easier to
follow if the CL had been assembled differently.

I guess you moved most of this file into the new test_rendering.py, and made
some modifications to that code after moving it... maybe next time, split it
into two CLs?

1. move the code to another file with absolutely minimal changes to keep it
working (making sure to use "svn cp" or "svn mv", to preserve history)
2. modify the code once it is in its new home

https://codereview.appspot.com/6611050/diff/3001/tools/test_pictures.py
File tools/test_pictures.py (right):

https://codereview.appspot.com/6611050/diff/3001/tools/test_pictures.py#newco...
tools/test_pictures.py:18: import test_rendering
Please separate this import from the others (the others, as noted, are common
Python modules)

See
https://code.google.com/p/skia/source/browse/trunk/tools/compare_baselines.py
for an example...

https://codereview.appspot.com/6611050/diff/3001/tools/test_pictures.py#newco...
tools/test_pictures.py:82: extra_args += ' --mode %s' % ' '.join(options.mode)
Does this work if options.mode has multiple values?  It seems to me like only
the first one would be associated with the "--mode" flag, and the others might
be abandoned.

Maybe use ' --mode "%s"' to put quotes around the complete list, or pass the
arguments as a list rather than a string?  (See my comments in the other file)

Edit: I see now that this code already existed elsewhere in this file.  So don't
feel obligated to do much about this... might be worth checking, though.

https://codereview.appspot.com/6611050/diff/3001/tools/test_rendering.py
File tools/test_rendering.py (right):

https://codereview.appspot.com/6611050/diff/3001/tools/test_rendering.py#newc...
tools/test_rendering.py:64: command = '%s %s %s' % (renderer_path,
inputs_as_string, render_dir)
Consider passing the complete command into RunCommand as a list of parameters...

RunCommand([renderer_path, inputs_as_string, render_dir] + args)

One nice thing about doing it this way is that (IIRC) it properly handles
parameters that contain spaces.  Also, you get to keep the data in a more
cleanly structured representation along the way.

See https://code.google.com/p/skia/source/browse/trunk/tools/svn.py for
examples.

Eric, you and I have gone back and forth about this in recent CLs... do you have
a strong preference at this point?


Edit: I see now that this code already existed in the other file.  So don't
feel obligated to do much about this... might be worth looking into a bit,
though.

[EricB, 2012-10-08 18:06:34]

https://codereview.appspot.com/6611050/diff/3001/tools/test_pictures.py
File tools/test_pictures.py (right):

https://codereview.appspot.com/6611050/diff/3001/tools/test_pictures.py#newco...
tools/test_pictures.py:29: """Parses the --mode option of the commandline.
Why are we using a 4-space indent?

https://codereview.appspot.com/6611050/diff/3001/tools/test_rendering.py
File tools/test_rendering.py (right):

https://codereview.appspot.com/6611050/diff/3001/tools/test_rendering.py#newc...
tools/test_rendering.py:64: command = '%s %s %s' % (renderer_path,
inputs_as_string, render_dir)
On 2012/10/05 19:54:14, epoger wrote:
> Consider passing the complete command into RunCommand as a list of
parameters...
> 
> RunCommand([renderer_path, inputs_as_string, render_dir] + args)
> 
> One nice thing about doing it this way is that (IIRC) it properly handles
> parameters that contain spaces.  Also, you get to keep the data in a more
> cleanly structured representation along the way.
> 
> See https://code.google.com/p/skia/source/browse/trunk/tools/svn.py for
> examples.
> 
> Eric, you and I have gone back and forth about this in recent CLs... do you
have
> a strong preference at this point?
> 
> 
> Edit: I see now that this code already existed in the other file.  So don't
> feel obligated to do much about this... might be worth looking into a bit,
> though.

Looks like RunCommand uses os.system.  I think subprocess.Popen is preferred. 
Popen supports the command being a string iff shell=True is also provided, which
is frowned upon unless absolutely necessary, so I like to use Popen with the
command in list form unless I need more advanced shell functionality.

[edisonn, 2012-10-09 13:04:43]

https://codereview.appspot.com/6611050/diff/3001/tools/test_pictures.py
File tools/test_pictures.py (right):

https://codereview.appspot.com/6611050/diff/3001/tools/test_pictures.py#newco...
tools/test_pictures.py:18: import test_rendering
On 2012/10/05 19:54:14, epoger wrote:
> Please separate this import from the others (the others, as noted, are common
> Python modules)
> 
> See
> https://code.google.com/p/skia/source/browse/trunk/tools/compare_baselines.py
> for an example...

Done.

https://codereview.appspot.com/6611050/diff/3001/tools/test_pictures.py#newco...
tools/test_pictures.py:29: """Parses the --mode option of the commandline.
I hav no idea, this file was added long before I joined the team. I maintained
consistency and continued with 4-space indent
On 2012/10/08 18:06:34, EricB wrote:
> Why are we using a 4-space indent?

[edisonn, 2012-10-09 13:32:49]

https://codereview.appspot.com/6611050/diff/3001/tools/test_pictures.py
File tools/test_pictures.py (right):

https://codereview.appspot.com/6611050/diff/3001/tools/test_pictures.py#newco...
tools/test_pictures.py:82: extra_args += ' --mode %s' % ' '.join(options.mode)
seem to work fine. I will preserve current functionality.
I don't like to mix refactoring with fixes/improvements

The parameters are passed correctly to render_pictures
dhcp-172-29-92-74:tools edisonn$ python ./test_pictures.py ./ ./ --mode pow2tile
128 128
running command [/Users/edisonn/src/skia/trunk/out/Debug/render_pictures ./
/var/folders/00/0thk0000h01000cxqpysvccm003_28/T/tmpJNZkvx --mode pow2tile 128
128]...

 

On 2012/10/05 19:54:14, epoger wrote:
> Does this work if options.mode has multiple values?  It seems to me like only
> the first one would be associated with the "--mode" flag, and the others might
> be abandoned.
> 
> Maybe use ' --mode "%s"' to put quotes around the complete list, or pass the
> arguments as a list rather than a string?  (See my comments in the other file)
> 
> Edit: I see now that this code already existed elsewhere in this file.  So
don't
> feel obligated to do much about this... might be worth checking, though.