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

Improvement: Base FixtureArgKeys on param values if possible, not param indices #11271

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sadra-barikbin
Copy link
Contributor

To base fixture argkeys on param values rather than on indices unless the value isn't hashable in which case we fallback to param index.

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sadra-barikbin, the implementation is very clean :)

This has come up previously in #9350. The main worry is about users using very large parametrize values. Arguably Hashable would exclude such values (such as numpy.array or large dicts) and fall back to the fast param index comparison. But I'm still pretty sure that we'll get "you made pytest slow" complaints from users using big hashable values like tuples etc, which we will now start hashing and comparing by value.

That's why I'm a bit scared to merge this as-is. After discussions in #9350, @haxtibal came up with a refinement of the idea in #9420, which keeps the Hashable business but adds an opt out for users who get hit by the "slow hashable value" problem. The workaround is to add a third comparison method, this one by parameter ID.

Unfortunately I dropped the ball on #9420 but there was fruitful discussion there.

Let me know what you think.

@sadra-barikbin
Copy link
Contributor Author

@bluetech , how is to compute the hash values only once using this wrapper around param value?

class _ParamValue:
    def __init__(self, value: Hashable):
        self.value = value
    
    @property
    def hash(self):
        return hash(self.value)
    
    def __hash__(self) -> int:
        return self.hash
    
    def __eq__(self, o: "_ParamValue") -> bool:
        return self.hash == o.hash

@dataclasses.dataclass(frozen=True)
class FixtureArgKeyByValue:
    argname: str
    param_value: _ParamValue
    scoped_item_path: Optional[Path]
    item_cls: Optional[type]

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.

2 participants