Fix vacuous test for private Hugging Face repo download without a token - #539
Open
MaxGhenis wants to merge 2 commits into
Open
Fix vacuous test for private Hugging Face repo download without a token#539MaxGhenis wants to merge 2 commits into
MaxGhenis wants to merge 2 commits into
Conversation
test_download_private_repo_no_token wrapped mock_download.assert_not_called() inside pytest.raises(Exception). download_huggingface_dataset never raises when hf_hub_download is mocked, so control reached assert_not_called(), whose AssertionError (the download had been called) satisfied pytest.raises. The test therefore passed regardless of what the code did, from its first commit (a7b1e51) onward. The intended behaviour, per #422, is that a private repo with no token in a non-interactive environment passes token=None through to hf_hub_download without prompting or raising; huggingface_hub then falls back to its own cached token (HF_TOKEN or the login file) and raises its own error if that is absent too. Rewrite the test to assert exactly that, parametrised over an unset and an empty HUGGING_FACE_TOKEN. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
- Give the getpass mock a return value so the "prompts when non-interactive" regression fails on mock_getpass.assert_not_called() instead of on a TypeError from storing a MagicMock in os.environ. - Add an HF_TOKEN-only parametrisation: core still passes token=None, pinning that resolving huggingface_hub's own cached token is huggingface_hub's job, not core's. - Assert the hf_hub_download return value is passed through. - Say precisely why the old test passed: control reached assert_not_called() only because the download had been called, so the test passed on the opposite of what its name claimed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
TestHuggingFaceDownload::test_download_private_repo_no_tokenintests/core/tools/test_hugging_face.pypassed vacuously. It did:download_huggingface_datasetdoes not raise (the download is mocked), so control reachedassert_not_called(), which raisedAssertionErrorbecause the download was called, and thatAssertionErrorsatisfiedpytest.raises(Exception). The test asserted nothing about the code. It has been this way since it was added in a7b1e51 (Dec 2024).Intended behaviour
Read from
download_huggingface_datasetandget_or_prompt_hf_token, and from #422:HUGGING_FACE_TOKENunset or empty and stdin not a TTY,get_or_prompt_hf_token()returnsNonerather than prompting (Handle empty HUGGING_FACE_TOKEN gracefully #422 made this deliberate so Dependabot CI without secrets does not hang or sendBearer).download_huggingface_datasetpasses thatNonethrough tohf_hub_downloadand does not raise.utils._headers.get_token_to_send) treatstoken=Noneas "use the cached token if any":HF_TOKENor thehf auth loginfile. If that is also missing, the Hub returns its own 401/GatedRepoError.So a hard failure in core when no
HUGGING_FACE_TOKENis set would break users who are logged in viahf auth loginand CI that relies onHF_TOKEN. This PR asserts the pass-through behaviour rather than adding a hard failure.Change
Test-only. The rewritten test, parametrised over
HUGGING_FACE_TOKENunset,HUGGING_FACE_TOKEN="", and huggingface_hub's ownHF_TOKENset withHUGGING_FACE_TOKENunset, exercises the realget_or_prompt_hf_tokenwithos.isattypatched toFalseand asserts:getpassis never called,hf_hub_downloadis called exactly once withtoken=None(and the other expected arguments), so in theHF_TOKENcase core leaves resolving that token to huggingface_hub,Changelog fragment:
changelog.d/fix-hf-no-token-test.fixed.md.Mutation check
Each mutation was applied to
policyengine_core/tools/hugging_face.pyin turn, then both the old test (frommaster) and the new test were run:RuntimeErrorwhen the resolved token isNone""instead ofNonetohf_hub_downloadgetpasseven when non-interactivemock_getpass.assert_not_called())HF_TOKENitself instead of leaving it to huggingface_hubhf-token-onlycase)hf_hub_downloadat allThe last row is the tell: the old test was not just vacuous but inverted. It passed whenever the download raised or
hf_hub_downloadwas called, and failed only in the one state its name described.Checks run
uvx ruff format .(no changes) anduvx ruff check .(clean)uv run pytest tests/core/tools/test_hugging_face.py -v: 17 passeduv run pytest testslocally: 697 passed, 4 skipped, 1 xfailedNot run:
make documentationandmake build(no docs or packaging changes).Out of scope
The review also suggested a
warnings.warnindownload_huggingface_datasetwhen a private or gated repo resolvestoken=None, so the eventual 401 is easier to diagnose than in #529. That is a behaviour change and belongs in its own PR.Note on #538
#538 ("Send HUGGING_FACE_TOKEN for public but gated Hugging Face repos") adds tests immediately after the block this PR rewrites, so whichever of the two merges second will need a rebase. The two do not overlap in intent: #538 adds a gated-repo variant of the same non-interactive-no-token check; this PR fixes the private-repo variant that already existed.
🤖 Generated with Claude Code