From 73905608264c0ef020a63054f42619b3c00ddec7 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 3 Sep 2026 15:26:18 -0400 Subject: [PATCH 1/2] Fix vacuous no-token Hugging Face download test 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 (a7b1e51c) 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 --- changelog.d/fix-hf-no-token-test.fixed.md | 1 + tests/core/tools/test_hugging_face.py | 74 ++++++++++++++++------- 2 files changed, 52 insertions(+), 23 deletions(-) create mode 100644 changelog.d/fix-hf-no-token-test.fixed.md diff --git a/changelog.d/fix-hf-no-token-test.fixed.md b/changelog.d/fix-hf-no-token-test.fixed.md new file mode 100644 index 00000000..01738858 --- /dev/null +++ b/changelog.d/fix-hf-no-token-test.fixed.md @@ -0,0 +1 @@ +Fix `test_download_private_repo_no_token`, which passed on its own `AssertionError` inside `pytest.raises(Exception)` and so checked nothing, so it now asserts that a private repo with no token available non-interactively passes `token=None` to `hf_hub_download` without prompting or raising. diff --git a/tests/core/tools/test_hugging_face.py b/tests/core/tools/test_hugging_face.py index 2a68682e..d19c9adb 100644 --- a/tests/core/tools/test_hugging_face.py +++ b/tests/core/tools/test_hugging_face.py @@ -77,35 +77,63 @@ def test_download_private_repo(self): local_dir=test_dir, ) - def test_download_private_repo_no_token(self): - """Test handling of private repo with no token""" + @pytest.mark.parametrize( + "environ", + [{}, {"HUGGING_FACE_TOKEN": ""}], + ids=["token-unset", "token-empty"], + ) + def test_download_private_repo_no_token(self, environ): + """Private repo, no token, non-interactive: token=None, no prompt, no raise. + + With HUGGING_FACE_TOKEN unset (or empty, as when Dependabot runs CI + without secrets), get_or_prompt_hf_token returns None instead of + prompting, and download_huggingface_dataset passes token=None through + to hf_hub_download rather than raising. huggingface_hub then falls + back to its own cached token (HF_TOKEN or the `hf auth login` file) + and raises its own error if that is missing too, so core must not + fail early here. + + The previous version of this test wrapped assert_not_called() inside + pytest.raises(Exception); the download never raised, so the + AssertionError from assert_not_called() satisfied pytest.raises and + the test passed without checking anything. + """ test_repo = "test_repo" test_filename = "test_filename" test_version = "test_version" test_dir = "test_dir" - with patch( - "policyengine_core.tools.hugging_face.hf_hub_download" - ) as mock_download: - with patch( - "policyengine_core.tools.hugging_face.model_info" - ) as mock_model_info: - mock_response = MagicMock() - mock_response.status_code = 404 - mock_response.headers = {} - mock_model_info.side_effect = RepositoryNotFoundError( - "Test error", response=mock_response - ) + with patch.dict(os.environ, environ, clear=True): + with patch("os.isatty", return_value=False): with patch( - "policyengine_core.tools.hugging_face.get_or_prompt_hf_token" - ) as mock_token: - mock_token.return_value = "" - - with pytest.raises(Exception): - download_huggingface_dataset( - test_repo, test_filename, test_version, test_dir - ) - mock_download.assert_not_called() + "policyengine_core.tools.hugging_face.getpass" + ) as mock_getpass: + with patch( + "policyengine_core.tools.hugging_face.hf_hub_download" + ) as mock_download: + with patch( + "policyengine_core.tools.hugging_face.model_info" + ) as mock_model_info: + mock_response = MagicMock() + mock_response.status_code = 404 + mock_response.headers = {} + mock_model_info.side_effect = RepositoryNotFoundError( + "Test error", response=mock_response + ) + + download_huggingface_dataset( + test_repo, test_filename, test_version, test_dir + ) + + mock_getpass.assert_not_called() + mock_download.assert_called_once_with( + repo_id=test_repo, + repo_type="model", + filename=test_filename, + revision=test_version, + token=None, + local_dir=test_dir, + ) class TestGetOrPromptHfToken: From 002b2c0d94465d669233ff07ea8352d3076eb443 Mon Sep 17 00:00:00 2001 From: Max Ghenis Date: Thu, 3 Sep 2026 15:39:20 -0400 Subject: [PATCH 2/2] Tighten the no-token test after review - 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 --- changelog.d/fix-hf-no-token-test.fixed.md | 2 +- tests/core/tools/test_hugging_face.py | 20 +++++++++++++------- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/changelog.d/fix-hf-no-token-test.fixed.md b/changelog.d/fix-hf-no-token-test.fixed.md index 01738858..65829b0d 100644 --- a/changelog.d/fix-hf-no-token-test.fixed.md +++ b/changelog.d/fix-hf-no-token-test.fixed.md @@ -1 +1 @@ -Fix `test_download_private_repo_no_token`, which passed on its own `AssertionError` inside `pytest.raises(Exception)` and so checked nothing, so it now asserts that a private repo with no token available non-interactively passes `token=None` to `hf_hub_download` without prompting or raising. +Fix `test_download_private_repo_no_token`, which passed only because `hf_hub_download` was called and its `assert_not_called()` raised inside `pytest.raises(Exception)`, so it now asserts that a private repo with no token available non-interactively passes `token=None` to `hf_hub_download` without prompting or raising. diff --git a/tests/core/tools/test_hugging_face.py b/tests/core/tools/test_hugging_face.py index d19c9adb..8740eb40 100644 --- a/tests/core/tools/test_hugging_face.py +++ b/tests/core/tools/test_hugging_face.py @@ -79,8 +79,8 @@ def test_download_private_repo(self): @pytest.mark.parametrize( "environ", - [{}, {"HUGGING_FACE_TOKEN": ""}], - ids=["token-unset", "token-empty"], + [{}, {"HUGGING_FACE_TOKEN": ""}, {"HF_TOKEN": "hf_cached_token"}], + ids=["token-unset", "token-empty", "hf-token-only"], ) def test_download_private_repo_no_token(self, environ): """Private repo, no token, non-interactive: token=None, no prompt, no raise. @@ -91,12 +91,16 @@ def test_download_private_repo_no_token(self, environ): to hf_hub_download rather than raising. huggingface_hub then falls back to its own cached token (HF_TOKEN or the `hf auth login` file) and raises its own error if that is missing too, so core must not - fail early here. + fail early here. The third case sets only huggingface_hub's own + HF_TOKEN: core still passes token=None, because resolving that token + is huggingface_hub's job, not core's. The previous version of this test wrapped assert_not_called() inside - pytest.raises(Exception); the download never raised, so the - AssertionError from assert_not_called() satisfied pytest.raises and - the test passed without checking anything. + pytest.raises(Exception). The download never raised, so control + reached assert_not_called(), whose AssertionError (the download had + been called) satisfied pytest.raises. The test passed precisely + because the download was called, the opposite of what its name + claimed to check. """ test_repo = "test_repo" test_filename = "test_filename" @@ -108,6 +112,7 @@ def test_download_private_repo_no_token(self, environ): with patch( "policyengine_core.tools.hugging_face.getpass" ) as mock_getpass: + mock_getpass.return_value = "prompted_token" with patch( "policyengine_core.tools.hugging_face.hf_hub_download" ) as mock_download: @@ -121,10 +126,11 @@ def test_download_private_repo_no_token(self, environ): "Test error", response=mock_response ) - download_huggingface_dataset( + result = download_huggingface_dataset( test_repo, test_filename, test_version, test_dir ) + assert result is mock_download.return_value mock_getpass.assert_not_called() mock_download.assert_called_once_with( repo_id=test_repo,