[Tests] fix initialization of kv hash in tests

[mickaelseznec]

Purpose

The tests pass, only because they run sequentially and the first do the necessary init. Now we can run them in any order (and parallel).

Test Plan

Test Result

---

Essential Elements of an Effective PR Description Checklist

• The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
• The test plan, such as providing test command.
• The test results, such as pasting the results comparison before and after, or e2e results
• (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
• (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

[gemini-code-assist]

Code Review

This pull request addresses an issue with test isolation by introducing a with_init_none_hash pytest fixture to ensure the KV cache hash is initialized before tests that require it. This is a good improvement for test stability and allows for parallel execution. The fixture is applied to many tests across the suite. My review identifies a couple of minor cases of redundancy where init_none_hash is called explicitly within tests that also use the new fixture. Removing these redundant calls will clean up the code.

[mickaelseznec]

cc @vMaroon

[vMaroon]

This definitely is a step towards cleaner testing, though it may look out of place when passed as test argument without much context.

Perhaps this can be taken a step further by using the @pytest.fixture(autouse=True) decorator and either:

1. Implicitly calling init_none_hash where needed:

pytest.fixture(autouse=True)
def _auto_init_hash_fn(request):
    if "hash_fn" in request.fixturenames:
        init_none_hash(request.getfixturevalue("hash_fn"))

2. Explicit dependency

@pytest.fixture
def initialized_hash_fn(hash_fn):
    """Fixture that automatically initializes the hash function."""
    init_none_hash(hash_fn)
    return hash_fn

# Update relevant tests to use the above fixture, e.g.:
@pytest.mark.parametrize("hash_fn", [sha256, sha256_cbor_64bit, hash])
def test_hash_block_tokens(initialized_hash_fn):
    # Replace the manual init_none_hash call
    hash_fn = initialized_hash_fn
    # ...

@mickaelseznec What do you think?

Both can optionally be enhanced by capturing and restoring global state by utilizing yield.
I think either is good, personal preference is to the first. I assume you tagged me because I introduced the manual inits, but maintainer input is required here (cc @heheda12345).

[mickaelseznec]

Thanks a lot for the feedback! The first proposition looks good, let me update my PR :)

[heheda12345]

I think we can keep the thing simple. For example, auto-apply with_init_none_hash in test_prefix_caching.py and call init_none_hash manually in other tests if needed. This won't be too confusing as most tests require it is related to hash.

I don't what it to be a autouse by default because then CI can't detect problems due to un-initialized non-hash.

[mickaelseznec]

Thanks for the nice ideas, I think I've come up with nice in-between:

• autouse fixture for the two relevant files only
  It has some code dup, but it's less intrusive (and probably easier to understand to read) than putting the fixture in conftest.py.

Let me know if that sounds good to you :)

[heheda12345]

Yeah make sense to me.

[mergify]

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @mickaelseznec.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

[heheda12345]

LGTM!

[heheda12345]

LGTM!