assertSameFiles of perceptualdiff engine fails when file paths contain spaces

[sterago]

First of all, thanks for the nice tool!

We are using it in a Jenkins CI setup, and the screenshots path happens to have spaces.

This is causing the perceptualdiff command to parse the --output parameter incorrectly, splitting it in correspondence of the space.

We've now worked around the issue by patching this line: https://github.com/bfirsh/needle/blob/master/needle/engines/perceptualdiff_engine.py#L20

specifically replacing

%s %s %s

with

'%s' '%s' '%s'

[acdha]

Hmmm, that entire line should really just be a list so subprocess will handle escaping for us.

[jphalip]

Thanks for the report. @sterago, could you confirm if this patch below would fix your issue?

diff --git a/needle/engines/perceptualdiff_engine.py b/needle/engines/perceptualdiff_engine.py
index 33ef157..bdf1777 100644
--- a/needle/engines/perceptualdiff_engine.py
+++ b/needle/engines/perceptualdiff_engine.py
@@ -17,8 +17,7 @@ class Engine(EngineBase):
         threshold = int(width * height * threshold)

         diff_ppm = output_file.replace(".png", ".diff.ppm")
-        cmd = "%s -threshold %d -output %s %s %s" % (
-            self.perceptualdiff_path, threshold, diff_ppm, baseline_file, output_file)
+        cmd = [self.perceptualdiff_path, '-threshold', threshold, '-output', diff_ppm, baseline_file, output_file]
         process = subprocess.Popen(cmd, shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
         perceptualdiff_stdout, _ = process.communicate()

[sterago]

hi @jphalip and thanks for the patch. This didn't work right away, and I had to change the following:

• the threshold argument needs to be a string, so I cast it to string: str(threshold) otherwise I get this error: http://stackoverflow.com/questions/20624342/typeerror-execv-arg-2-must-contain-only-strings
• the shell argument of the Popen constructor had to be set to False, otherwise I would get a syntax error from the perceptual_diff executable

other than that, it works fine

hope this helps