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

Improve discrimination of database keys #3867

Open
jobh opened this issue Jan 30, 2024 · 5 comments
Open

Improve discrimination of database keys #3867

jobh opened this issue Jan 30, 2024 · 5 comments
Labels
enhancement it's not broken, but we want it to be better performance go faster! use less memory!

Comments

@jobh
Copy link
Contributor

jobh commented Jan 30, 2024

We don't (and can't) guarantee that our database keys are unique. If two test methods have identical keys, the consequences are limited: We replay arbitrary examples instead of previously-failing ones. This is a low-priority issue, but recorded here as a reminder.

While the downsides are limited, they do exist: a slight slowdown plus loss of the database benefits. Hence, we should try hard (within reason) to actually generate unique-yet-stable database keys.

Some ideas are:

One common failure mode in our own tests is inner tests used in multiple contexts, either by parametrized strategies or inside a helper function (tests.common.debug).

  • For the parametrized strategies, we might mix in the @given arguments (if we recognize the need - see Clean up use of example in tests #3865 (comment)).
  • Plus, mix in __qualname__ because inner tests are often called... inner.
  • This might not be enough for the helper functions though, as they typically also take an asserted predicate as input — for those, maybe a @database_key decorator where we can mix in the predicate as well? Do note, we already set database=None in at least some of these, which fixes the slowdown issue.
@jobh jobh added enhancement it's not broken, but we want it to be better performance go faster! use less memory! labels Jan 30, 2024
@Zac-HD
Copy link
Member

Zac-HD commented Jan 30, 2024

I agree that we should pick up any low-hanging improvements here, and this list looks pretty good to me.

Specific to the last point, we have a magic attribute used to distinguish @pytest.mark.parametrize()d tests, which could also be used here. Just assign whatever bytestring you like, and it'll be hashed into the database key along with everything else, e.g.:

# Give every parametrized test invocation a unique database key
key = item.nodeid.encode()
item.obj.hypothesis.inner_test._hypothesis_internal_add_digest = key

@jobh
Copy link
Contributor Author

jobh commented Jan 30, 2024

Hmm, I think we might add to the list

  • Pick up _hypothesis_internal_add_digest from the currently executing test function instead of the function actually passed to given.

otherwise we'd duplicate keys for f.x.

@pytest.mark.parametrize("v", [1, 2])
def test_something(v):

    @given(st.integers(v))
    def inner(x):
        assert x >= v

    inner()

since the attribute is added to test_something and not inner. At that point, it might be more straightforward to just store the add_digest as a global/threadlocal rather than as an attribute.

[edit] ...actually, we could mix in currently executing nodeid regardless of parametrization... and call it a day. Too pytest specific?

@Zac-HD
Copy link
Member

Zac-HD commented Jan 31, 2024

[edit] ...actually, we could mix in currently executing nodeid regardless of parametrization... and call it a day. Too pytest specific?

Too specific - the problem is that if you execute a particular test function manually or via pytest, or a method with unittest or pytest, we want to have the same database key in either case. So mixing in the nodeid is fine when it's a parametrized test because there's a solid benefit and we're unlikely to execute it without pytest, but I'd prefer to avoid doing that unconditionally.

@jobh
Copy link
Contributor Author

jobh commented Jan 31, 2024

Understood, thanks! I have just one more question:

We don't use the strategy definition in our db key, and it looks intentional (e.g., _clean_source in get_digest and ignoring specifier in find()). This leads to collisions in find (naturally), and also f.x. test_attrs_inference_builds which is defined by the same name but slightly different strategy in two test modules.

Why is this? Is it beneficial to have key stability across changes in strategy?

@Zac-HD
Copy link
Member

Zac-HD commented Jan 31, 2024

It's actually pretty hard to derive a stable-across-runs bytestring from an arbitrary strategy; for example it's easy to end up with a repr that includes some object's memory address, or less often something like the current time or date or machine name or OS.

We strip the decorators from source code before hashing it so that adding or removing @settings() and @example() decorators doesn't affect the database key.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement it's not broken, but we want it to be better performance go faster! use less memory!
2 participants