Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return False when image diff file not found #1856

Merged
merged 1 commit into from
Nov 8, 2022

Conversation

wrma6
Copy link

@wrma6 wrma6 commented May 10, 2022

Description of Change(s)

In testWrapper.py _imageDiff, the test will pass if glob does not find the file. This change would make the test return False (fail) if this happens, so that the problem is not silently passed over. Also, in the _diff function in the same file, there is an unnecessary second call to glob, which this change would remove.

Fixes Issue(s)

  • _imageDiff function in testWrapper.py returns True (passes) when the file cannot be found
  • _diff function in testWrapper.py makes an unncessary repeat call to glob.glob
  • two tests that reference the wrong images have been fixed
  • one test (testUsdImagingGLInstancing_nestedInstance) has been disabled, the test itself is failing
  • I have submitted a signed Contributor License Agreement

@jilliene
Copy link

Filed as internal issue #USD-7368

@wrma6 wrma6 force-pushed the pr/fix-empty-imagediff branch from 13e2624 to 04fb37b Compare May 17, 2022 14:53
@pmolodo
Copy link
Contributor

pmolodo commented May 19, 2022

Note - we've commented out one test, testUsdImagingGLInstancing_nestedInstance. This actually isn't changing behavior, though, as the tests was effectively already being skipped, due to it looking for the wrong image to compare against (testUsdImagingGLInstancing_instance3.png, instead of testUsdImagingGLInstancing_nestedInstance.png.) Prior to this change, that would just result in the test being silently skipped.

However, once the test was fixed to compare the correct file, it exposed what looks like a true test failure. As a result, we've disabled the test for now. If you'd like, we can make a separate issue to fix + re-enable this test.

Baseline:
testUsdImagingGLInstancing_nestedInstance baseline

Rendered:
testUsdImagingGLInstancing_nestedInstance rendered

@wrma6 wrma6 force-pushed the pr/fix-empty-imagediff branch from 04fb37b to c681616 Compare July 7, 2022 18:02
@wrma6 wrma6 force-pushed the pr/fix-empty-imagediff branch from c681616 to 94b2a9b Compare July 7, 2022 18:29
@wrma6
Copy link
Author

wrma6 commented Jul 7, 2022

It seems that a change to testUsdImagingGLInstancing_nestedInstance has been merged while this PR was open, so I have removed the changes in this PR for that test.

@pmolodo
Copy link
Contributor

pmolodo commented Jul 7, 2022

Huzzah for the test getting fixed! Can we get this merged so future test failures like that won't slip through unnoticed?

@pmolodo
Copy link
Contributor

pmolodo commented Oct 6, 2022

Just checking in on this - it's not particularly urgent, but it's also a pretty simple fix, which helps ensure future correctness of tests. As shown above, there was already one test failure which slipped through, and perhaps would have been caught + fixed sooner if this change had been in place...

@sunyab
Copy link
Contributor

sunyab commented Oct 6, 2022

Hey @pmolodo , sorry for the delay on this one. I had this staged but went on leave before I could commit it. This won't make it in for the upcoming 22.11 release but will be in the next one. Thanks!

@pmolodo
Copy link
Contributor

pmolodo commented Oct 7, 2022 via email

@pixar-oss pixar-oss merged commit bf18190 into PixarAnimationStudios:dev Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants