Allow using multiple cache images

[user27182]

Resolve #197

Instead of a separate flaky_tests dir (as suggested in #197), this PR allows subdirectories inside the cache dir directly. To specify multiple images, just place them inside a sub-dir with the test name. This is consistent with the changes in #198.

This is also tested downstream in pyvista/pyvista#7914

[codecov-commenter]

Codecov Report

❌ Patch coverage is 97.40260% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.07%. Comparing base (ead15ee) to head (d744e3b).

Files with missing lines	Patch %	Lines
pytest_pyvista/pytest_pyvista.py	97.40%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #211      +/-   ##
==========================================
+ Coverage   95.34%   96.07%   +0.72%     
==========================================
  Files           2        2              
  Lines         258      306      +48     
  Branches       39       46       +7     
==========================================
+ Hits          246      294      +48     
+ Misses          6        5       -1     
- Partials        6        7       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[akaszynski]

One nitpick, otherwise LGTM.

[edabor]

Thanks for the feature.

IMO, it would be nice to allows users to customize the env_info, for example if they want to include their library version.

[user27182]

Thanks for the feature.

IMO, it would be nice to allows users to customize the env_info, for example if they want to include their library version.

This was sort of previously supported as a --generated_image_name arg that could be unspecified (use default env info) or provide a fully custom input. It was changed / renamed / removed in eb49e26

Is this the kind of customization you would prefer? I removed it because (1) the arg name wasn't great, since passing the flag generated_image_name without providing a name seemed confusing/ambiguous about what it meant, and (2) actually providing this info is challenging in CI (How to get vtk version and pass it as an arg?).

Can you provide more details about what kind of API you want for customizing this? We can add a separate flag for appending a prefix or suffix? Something like --subdir_image_name_suffix?

[edabor]

This was sort of previously supported as a --generated_image_name arg that could be unspecified (use default env info) or provide a fully custom input. It was changed / renamed / removed in eb49e26

Is this the kind of customization you would prefer? I removed it because (1) the arg name wasn't great, since passing the flag generated_image_name without providing a name seemed confusing/ambiguous about what it meant, and (2) actually providing this info is challenging in CI (How to get vtk version and pass it as an arg?).

Can you provide more details about what kind of API you want for customizing this? We can add a separate flag for appending a prefix or suffix? Something like --subdir_image_name_suffix?

Agreed that it can be tough to specify the prefix via command-line, my request was not clear sorry.
I'm 100% for leaving --generate_subdirs as a store_true.

I was more thinking of having an attribute to VerifyImageCache to enable such kind of customization:

@pytest.fixture(autouse=True)
def verify_image_cache_wrapper(verify_image_cache):
    import vtk, mylib
    verify_image_cache.subdir_image_prefix = f"{vtk.__version__}_{mylib.__version__}"
    return verify_image_cache

It would default to ENV_INFO if not provided.

[user27182]

Allowing customization in the fixture directly makes sense.

I am planning on adding more options for env info (e.g. check for GitHub RUNNER_ENVIRONMENT to append 'self-hosted', or include rendering info like EGL or GPU manufacturer). Maybe we can add these along with your request in a follow-up PR?

I am planning on adding --generate_subdirs option to --doc_mode as well from #198. How can we generalize your feature request to allow customization the name for doc mode as well, since we can't customize a fixture for that case?

Maybe we can try to address all of this and harmonize / extend the API after this PR and the doc mode PR merge?

[user27182]

I am planning on adding --generate_subdirs option to --doc_mode as well from #198. How can we generalize your feature request to allow customization the name for doc mode as well, since we can't customize a fixture for that case?

We could include a placeholder for a custom package version in the env info string. Then pass an arg --include_package_info (or some other better name) which allows you specify the package? E.g. --include_package_info geovista would do import geovista and append geovista.__version__ to the name

[edabor]

I am planning on adding more options for env info (e.g. check for GitHub RUNNER_ENVIRONMENT to append 'self-hosted', or include rendering info like EGL or GPU manufacturer). Maybe we can add these along with your request in a follow-up PR?

Good idea.

I am planning on adding --generate_subdirs option to --doc_mode as well from #198. How can we generalize your feature request to allow customization the name for doc mode as well, since we can't customize a fixture for that case?

Hard to tell. We could add a method to VerifyImageCache dedicated to run the image comparison.
Then, the fixture could be requested in doc mode and the method called within.

Maybe we can try to address all of this and harmonize / extend the API after this PR and the doc mode PR merge?

Agreed.

[user27182]

I am planning on adding --generate_subdirs option to --doc_mode as well from #198. How can we generalize your feature request to allow customization the name for doc mode as well, since we can't customize a fixture for that case?

Hard to tell. We could add a method to VerifyImageCache dedicated to run the image comparison.

Then, the fixture could be requested in doc mode and the method called within.

I am still unsure about how users would interface with this fixture though, since users can't request the fixture and add code easily in doc mode. Maybe we can add a hook so users can customize this in their conftest.py file?

Are you interested in customizing this on a per test (per image) basis? Or on a global basis (for all images)?

[edabor]

I would say it makes more sense to customize for all images, since when using subdirs, the test name is the folder itself.
Moreover, the platform and other env variables, which are useful to track the origin of the image, are constant throughout the whole pytest session, thus in favor of customizing for all images.