decorator hash_library kwarg option precedence

Within the plugin.ImageComparison.compare_image_to_hash_library method, should it not be the case that the mpl_image_compare marker hash_library kwarg option should have precedence over the --mpl-hash-library CLI argument?

pytest-mpl/pytest_mpl/plugin.py

Lines 520 to 521 in e387618

    
           hash_library_filename = self.hash_library or compare.kwargs.get('hash_library', None) 
        
           hash_library_filename = (Path(item.fspath).parent / hash_library_filename).absolute()

At the moment this isn't the case, however a similar precedence is supported for the baseline_dir decorator option e.g.,

I'm happy to take this on, if this is the case 👍

@ConorMacBride

I am not sure about this one. It seems to me that having the CLI override the one in the code makes more sense, as it would let a user specify a different test hash lib or something?

The fact it's inconsistent is more problematic. I am up for input from others here, @ConorMacBride @astrofrog

It would be more typical for a CLI argument to override the configured value. However, in this case a different hash_library (and a different baseline_dir) can be passed as a kwarg to each test, with the CLI argument being used as the default for kwargs not passed to a specific test.

So each test, in theory, could use a different hash library, and overriding the configured value would break this. That said, I think using multiple hash libraries is not tested and therefore not encouraged.

If you think we should support a different hash library and baseline directory for each test then I think kwarg should take priority. But if we are to require the same library and directory, I think the CLI argument should take priority.

@astrofrog

This feels like it is walking into a bigger discussion including some of the points raised in #149.

At the moment our API is very flexible in terms of what we test against, defined per-test in Python you can do pretty much anything you like based on installed package versions etc.

I think I would be pro reducing that complexity a little, and that includes one baseline_dir / hash library per test run. It would make it easier to configure (you could set some config option in conftest.py or at the CLI) and reduce the overhead.

That being said, we have provided the per-decorator argument since this package existed (right @astrofrog?) and there are people using it, so any change from that would have to be slow and throw many-a-warning.

I guess what would be ideal here is aiming for a consistent behaviour across pytest-mpl, whatever that is... but in order to do that, from what you're saying, that might be a breaking change for a major release.

If that's the case, are there more such changes that can be part of such a major release?

For example, if #150 lands, would you guys consider making phash the default kernel instead of sha256?

If so, then that would be a breaking change in behaviour, and that's a suitable change for a major release... 🤔

I agree consistency is a good goal.

Planning for a major breaking release also sounds like a good idea. I not sure if I will have the time to really sit down and audit the API, but if someone else wants to do it that would be great :D

Cool, I'll scrape through the code and attempt to summarise the current state of the API, then we can roll from there 👍

I agree consistency is a good goal.

Planning for a major breaking release also sounds like a good idea. I not sure if I will have the time to really sit down and audit the API, but if someone else wants to do it that would be great :D

I've reviewed the API in #198 and in #199, and have made some suggestions for changes. Suggested changes relevant to this issue are:

Deprecate the use of multiple hash libraries in the same pytest run (not necessary because files are small and include the full module path anyway).
Deprecate the use of multiple testing modes in the same pytest run (e.g. warn if one test only has hash comparison while the others only have image comparison)
--mpl-hash-library should be relative to where pytest was run (to match --mpl-baseline-path).
Local hash_library kwarg should take precedence over global --mpl-hash-library CLI option. (Should not matter given [1] but good for consistency with the baseline_dir kwarg.)
Add mpl-hash-library ini option (relative to where pytest was run). (not a breaking change)

For the two deprecations, I don't think we should go out of our way to prevent people doing these but we should detect these cases and warn the user that it is not supported. I do think we should support multiple baseline images directories as it can be nice to have a separate baseline image directory within each test module, and this is actually what pytest-mpl does by default currently.

I think it is useful to keep the hash_library decorator even if they all need to have the same value because some packages determine the name of the hash library depending on what version of Matplotlib and FreeType is installed.

Please let me know if you have any thoughts, either here or in #198

Navigation Menu

Repository navigation

decorator hash_library kwarg option precedence #154

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	hash_library_filename = self.hash_library or compare.kwargs.get('hash_library', None)
	hash_library_filename = (Path(item.fspath).parent / hash_library_filename).absolute()

decorator hash_library kwarg option precedence #154

Description

Activity

bjlittle commented on May 19, 2022

Cadair commented on May 20, 2022

ConorMacBride commented on May 20, 2022

Cadair commented on May 20, 2022

bjlittle commented on May 20, 2022

bjlittle commented on May 20, 2022

Cadair commented on May 20, 2022

bjlittle commented on May 20, 2022

ConorMacBride commented on Apr 9, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions