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

Need a test/routine for hash alignment between test data images and imagerepo.json #5539

Open
trexfeathers opened this issue Oct 10, 2023 · 7 comments

Comments

@trexfeathers
Copy link
Contributor

trexfeathers commented Oct 10, 2023

📰 Custom Issue

The Iris image tests create hashes of all the generated images, and compare with the stored hashes in imagerepo.json. This gives us our graphics test coverage 👍

idiff.py provides an important convenience to help developers check failures by viewing images side-by-side. For this we use the images held in iris-test-data. We request in the documentation that iris-test-data is kept up to date with the images that produce the passing hashes, but I was surprised to learn that there are no scripts that guarantee this link. We have a check in test_image_json.py to make sure iris-test-data and imagerepo.json both have the same list of images, but any/all of these stored hashes could be inconsistent and we would not know.

This situation risks a divergence, which we would not be aware of, and which would mean developers acting on false information when running idiff.py.

It would be easy enough to write something. But since this involves hashing all the images in iris-test-data I would be reluctant to include this in the CI tests that are run against pull requests - too much time/resource. We could run a scheduled check once a day/week?

@trexfeathers trexfeathers changed the title Need a test/routine that ensures alignment between test data images and imagerepo.json Need a test/routine for hash alignment between test data images and imagerepo.json Oct 10, 2023
@rcomer
Copy link
Member

rcomer commented Oct 10, 2023

Maybe it could run specifically in PRs that change imagerepo.json, which are not all that frequent.

@trexfeathers
Copy link
Contributor Author

@SciTools/peloton:

Image hashing is fast, and we are already downloading all of the images as part of iris-test-data. Team is in agreement that we could do with a test like this, but it even sounds like we could write a routine as part of the normal tests.

There were previous problems with rate limiting, but we suspect this is because the old world downloaded each image from GitHub individually and raised DDoS flags. So hopefully not a problem anymore.

@trexfeathers
Copy link
Contributor Author

(if there were a rate limiting problem we could write a CRON GHA within iris-test-data)

@rcomer
Copy link
Member

rcomer commented Oct 11, 2023

If we are thinking of hashing all the reference images as part of the regular tests, could we ditch the imagerepo.json entirely and just have the individual image tests compare directly to the reference images?

@trexfeathers
Copy link
Contributor Author

If we are thinking of hashing all the reference images as part of the regular tests, could we ditch the imagerepo.json entirely and just have the individual image tests compare directly to the reference images?

Instinctively that makes sense to me. However I'm acutely aware that @bjlittle and @wjbenfold talked around this topic several times so there might be a good reason I'm not thinking of

@wjbenfold
Copy link
Contributor

I'm pretty rusty on this but I've had a bit of a dig so hopefully these thoughts make sense:

imagerepo.json gives us - I think - a link from which Iris commit we're at (which defines versions of our dependencies too) and what the hash should be. That means we can test old versions of Iris (or patch updates to old releases) as well as the newest version. We can lose that and do things a different way as long as we're account for that use case (I think).

Doing the tests when we're using the test data anyway in the CI should be pretty easy as we could diff the results of tests/graphics/recreate_imagerepo.py against the current imagerepo.json (if I'm understanding how it works correctly)?

@ESadek-MO
Copy link
Contributor

@SciTools/peloton @wjbenfold Thanks for confirming our understanding, we agree it should be pretty straightforward.

@scitools-ci scitools-ci bot removed this from 🚴 Peloton Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

No branches or pull requests

4 participants