[testing] output failed diffs (image or text) to a common dir

Diagnosing issues with failed diffs (particularly image diffs) can be
difficult because the generated output files are in a randomly named
temporary directory, which can be difficult to locate.

With this change, for each failed diff, these files are created:

${CMAKE_BINARY_DIR}/Testing/Failed-Diffs/${TEST_NAME}/${filename}.result.${ext}
${CMAKE_BINARY_DIR}/Testing/Failed-Diffs/${TEST_NAME}/${filename}.baseline.${ext}

CMakeLists.txt

@@ -56,6 +56,10 @@ if (PXR_BUILD_PRMAN_PLUGIN)
     endif()
 endif()
 
+if (PXR_BUILD_TESTS)
+    pxr_tests_prologue()
+endif()
+
 if (PXR_BUILD_DOCUMENTATION)
     pxr_build_documentation()
 endif()   

cmake/defaults/CTestCustom.cmake

@@ -0,0 +1,9 @@
+# set a timestamp id to identify this run of tests in the environment, if not
+# already set
+if (NOT DEFINED ENV{PXR_CTEST_RUN_ID})
+    # otherwise, 
+    # Note - can't use default format for TIMESTAMP, as it contains ":", which
+    # isn't allowed in windows filepaths
+    string(TIMESTAMP _current_time "%Y-%m-%dT%H.%M.%S")
+    set(ENV{PXR_CTEST_RUN_ID} ${_current_time})
+endif()

cmake/macros/Public.cmake

@@ -723,12 +723,6 @@ function(pxr_register_test TEST_NAME)
         foreach(compareFile ${bt_DIFF_COMPARE})
             set(testWrapperCmd ${testWrapperCmd} --diff-compare=${compareFile})
         endforeach()
-
-        # For now the baseline directory is assumed by convention from the test
-        # name. There may eventually be cases where we'd want to specify it by
-        # an argument though.
-        set(baselineDir ${testenvDir}/baseline)
-        set(testWrapperCmd ${testWrapperCmd} --baseline-dir=${baselineDir})
     endif()
 
     if (bt_IMAGE_DIFF_COMPARE)
@@ -737,12 +731,6 @@ function(pxr_register_test TEST_NAME)
                 set(testWrapperCmd ${testWrapperCmd} --image-diff-compare=${compareFile})
             endforeach ()
 
-            # For now the baseline directory is assumed by convention from the test
-            # name. There may eventually be cases where we'd want to specify it by
-            # an argument though.
-            set(baselineDir ${testenvDir}/baseline)
-            set(testWrapperCmd ${testWrapperCmd} --baseline-dir=${baselineDir})
-
             if (bt_WARN)
                 set(testWrapperCmd ${testWrapperCmd} --warn=${bt_WARN})
             endif()
@@ -778,6 +766,21 @@ function(pxr_register_test TEST_NAME)
         endif()
     endif()
 
+    if (bt_DIFF_COMPARE OR bt_IMAGE_DIFF_COMPARE)
+        # Common settings we only want to set once if either is used
+
+        # For now the baseline directory is assumed by convention from the test
+        # name. There may eventually be cases where we'd want to specify it by
+        # an argument though.
+        set(baselineDir ${testenvDir}/baseline)
+        set(testWrapperCmd ${testWrapperCmd} --baseline-dir=${baselineDir})
+
+        # <PXR_CTEST_RUN_ID> will be set by CTestCustom.cmake, and then
+        # expanded by testWrapper.py
+        set(failuresDir ${CMAKE_BINARY_DIR}/Testing/Failed-Diffs/<PXR_CTEST_RUN_ID>/${TEST_NAME})
+        set(testWrapperCmd ${testWrapperCmd} --failures-dir=${failuresDir})
+    endif()
+
     if (bt_CLEAN_OUTPUT)
         set(testWrapperCmd ${testWrapperCmd} --clean-output-paths=${bt_CLEAN_OUTPUT})
     endif()
@@ -1235,3 +1238,19 @@ function(pxr_core_epilogue)
         set(_building_core FALSE PARENT_SCOPE)
     endif()
 endfunction() # pxr_core_epilogue
+
+function(pxr_tests_prologue)
+    add_custom_target(
+        test_setup
+        ALL
+        DEPENDS "${CMAKE_BINARY_DIR}/CTestCustom.cmake"
+    )
+    add_custom_command(
+        OUTPUT "${CMAKE_BINARY_DIR}/CTestCustom.cmake"
+        COMMAND ${CMAKE_COMMAND} -E copy
+            "${CMAKE_CURRENT_SOURCE_DIR}/cmake/defaults/CTestCustom.cmake"
+            "${CMAKE_BINARY_DIR}/CTestCustom.cmake"
+        DEPENDS "${CMAKE_CURRENT_SOURCE_DIR}/cmake/defaults/CTestCustom.cmake"
+        COMMENT "Copying CTestCustom.cmake"
+    )
+endfunction() # pxr_tests_prologue

cmake/macros/testWrapper.py

@@ -98,6 +98,13 @@ def _parseArgs():
             help='Testenv directory to copy into test run directory')
     parser.add_argument('--baseline-dir',
             help='Baseline directory to use with --diff-compare')
+    parser.add_argument('--failures-dir',
+            help=('If either --diff-compare or --image-diff-compare fail, then '
+                  'copy both the generated and baseline images into this '
+                  'directory; if <PXR_CTEST_RUN_ID>" is in the value, it is '
+                  'replaced with a timestamp identifying a given ctest '
+                  'invocation'))
+
     parser.add_argument('--expected-return-code', type=int, default=0,
             help='Expected return code of this test.')
     parser.add_argument('--env-var', dest='envVars', default=[], type=str, 
@@ -150,14 +157,18 @@ def _windowsReplacement(m):
         inputFile.write(data)
         inputFile.truncate()
 
+def _addFilenameSuffix(path, suffix):
+    base, ext = os.path.splitext(path)
+    return base + suffix + ext
+
 def _cleanOutput(pathPattern, fileName, verbose):
     if verbose:
         print("stripping path pattern {0} from file {1}".format(pathPattern, 
                                                                 fileName))
     _stripPath(fileName, pathPattern)
     return True
 
-def _diff(fileName, baselineDir, verbose):
+def _diff(fileName, baselineDir, verbose, failuresDir=None):
     # Use the diff program or equivalent, rather than filecmp or similar
     # because it's possible we might want to specify other diff programs
     # in the future.
@@ -174,19 +185,22 @@ def _diff(fileName, baselineDir, verbose):
         return False
 
     for fileToDiff in glob.glob(fileName):
-        cmd = [diff, _resolvePath(baselineDir, fileToDiff), fileToDiff]
+        baselineFile = _resolvePath(baselineDir, fileToDiff)
+        cmd = [diff, baselineFile, fileToDiff]
         if verbose:
             print("diffing with {0}".format(cmd))
 
         # This will print any diffs to stdout which is a nice side-effect
         if subprocess.call(cmd) != 0:
+            if failuresDir is not None:
+                _copyFailedDiffFiles(failuresDir, baselineFile, fileToDiff)
             return False
 
     return True
 
 def _imageDiff(fileName, baseLineDir, verbose, env, warn=None, warnpercent=None,
                hardwarn=None, fail=None, failpercent=None, hardfail=None,
-               perceptual=None):
+               perceptual=None, failuresDir=None):
     import platform
     if platform.system() == 'Windows':
         imageDiff = 'idiff.exe'
@@ -218,7 +232,8 @@ def _imageDiff(fileName, baseLineDir, verbose, env, warn=None, warnpercent=None,
     for image in glob.glob(fileName):
         cmd = [imageDiff]
         cmd.extend(cmdArgs)
-        cmd.extend([_resolvePath(baseLineDir, image), image])
+        baselineImage = _resolvePath(baseLineDir, image)
+        cmd.extend([baselineImage, image])
 
         if verbose:
             print("image diffing with {0}".format(cmd))
@@ -230,6 +245,8 @@ def _imageDiff(fileName, baseLineDir, verbose, env, warn=None, warnpercent=None,
         # 3: The images were not the same size and could not be compared.
         # 4: File error: could not find or open input files, etc.
         if subprocess.call(cmd, shell=False, env=env) not in (0, 1):
+            if failuresDir is not None:
+                _copyFailedDiffFiles(failuresDir, baselineImage, image)
             return False
 
     return True
@@ -244,7 +261,27 @@ def _copyTree(src, dest):
         if os.path.isdir(s):
             shutil.copytree(s, d)
         else:
-            shutil.copy2(s, d) 
+            shutil.copy2(s, d)
+
+def _copyFailedDiffFiles(failuresDir, baselineFile, resultFile):
+    baselineName = _addFilenameSuffix(os.path.basename(baselineFile),
+                                      ".baseline")
+    resultName = _addFilenameSuffix(os.path.basename(resultFile), ".result")
+    baselineOutpath = os.path.join(failuresDir, baselineName)
+    resultOutpath = os.path.join(failuresDir, resultName)
+
+    if not os.path.isdir(failuresDir):
+        os.makedirs(failuresDir)
+
+    shutil.copy(baselineFile, baselineOutpath)
+    shutil.copy(resultFile, resultOutpath)
+
+    print("Image diff failure:\n"
+          "  Copied:\n"
+          "    {baselineName}\n"
+          "    {resultName}\n"
+          "  Into:\n"
+          "    {failuresDir}".format(**locals()))
 
 # subprocess.call returns -N if the process raised signal N. Convert this
 # to the standard positive error code matching that signal. e.g. if the
@@ -379,9 +416,14 @@ def _runCommand(raw_command, stdout_redir, stderr_redir, env,
 
     # If desired, diff the provided file(s) (must be generated by the test somehow)
     # with a file of the same name in the baseline directory
+    failuresDir = None
+    if args.failures_dir:
+        failuresDir = args.failures_dir.replace('<PXR_CTEST_RUN_ID>',
+            os.environ.get('PXR_CTEST_RUN_ID', 'NOT_RUN_FROM_CTEST'))
     if args.diff_compare:
         for diff in args.diff_compare:
-            if not _diff(diff, args.baseline_dir, args.verbose):
+            if not _diff(diff, args.baseline_dir, args.verbose,
+                         failuresDir=failuresDir):
                 sys.stderr.write('Error: diff for {0} failed '
                                  '(DIFF_COMPARE).'.format(diff))
                 sys.exit(1)
@@ -392,6 +434,7 @@ def _runCommand(raw_command, stdout_redir, stderr_redir, env,
                   ('warn', 'warnpercent', 'hardwarn',
                    'fail', 'failpercent', 'hardfail', 'perceptual')
                   if key in converted}
+        params["failuresDir"] = failuresDir
 
         for image in args.image_diff_compare:
             if not _imageDiff(image, args.baseline_dir, args.verbose, env, **params):