[testing] output failed diffs (image or text) to a common dir #1839
Conversation
|
Filed as internal issue #USD-7338 |
There was a problem hiding this comment.
Thanks @pmolodo for the PR.
This generally looks fine to us. Could you please address few notes I am posting here from our internal review?
We also feel it will make sense to augment BUILDING.md to mention about where outputs from failed tests will be written.
cmake/defaults/Options.cmake
Outdated
| @@ -24,6 +24,9 @@ | |||
| option(PXR_STRICT_BUILD_MODE "Turn on additional warnings. Enforce all warnings as errors." OFF) | |||
| option(PXR_VALIDATE_GENERATED_CODE "Validate script generated code" OFF) | |||
| option(PXR_HEADLESS_TEST_MODE "Disallow GUI based tests, useful for running under headless CI systems." OFF) | |||
| option(PXR_OUTPUT_FAILED_TEST_DIFFS | |||
| "Copy failed diff/image diff tests into \${CMAKE_BINARY_DIR}/Testing/Failed-Diffs/<PXR_CTEST_RUN_ID>/\${TEST_NAME}. <PXR_CTEST_RUN_ID> is automatically set per ctest-invocation, or read from an environment variable of the same name if set." | |||
| OFF) | |||
There was a problem hiding this comment.
Is there a particular reason we wouldn't want to have this on all the time? We think it make sense to turn this on all the time for simplicity?
There was a problem hiding this comment.
Sure, I'm all for that! The only argument against having it on by default is the it means the build directory might grow over time - each test run with a failed image diff will result in a new directory in ${CMAKE_BINARY_DIR}/Testing/Failed-Diffs. However, that's no different (and much less than) the filesystem usage growth that already occurs in the temp directories (it's just more easily visible / discoverable). But I think the pros outweigh the cons - ideally you won't have failed tests too often, and only more "advanced" users are probably running the tests a lot anyway.
There was a problem hiding this comment.
| os.makedirs(failuresDir) | ||
|
|
||
| shutil.copy(baselineFile, baselineOutpath) | ||
| shutil.copy(resultFile, resultOutpath) |
There was a problem hiding this comment.
Sunya noticed that it WBN to print out some kind of message here saying where the diff results are being copied to. That way someone looking at the test output can see the exact path they need to go to to look for more info.
There was a problem hiding this comment.
Absolutely, will do!
There was a problem hiding this comment.
| @@ -379,9 +408,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>', | |||
There was a problem hiding this comment.
If PXR_CTEST_RUN_ID isn't set in the environment this will generate an invalid path with "<", ">" in it. It was recommended to just drop "<", ">"?
There was a problem hiding this comment.
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}
pmolodo
force-pushed
the
pr/failed_diff_output_dir
branch
from
4e1b0b3
to
c24838f
Compare
|
Responded to the code review notes, and pushed. To see just the incremental changes, look here: |
Description of Change(s)
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: