Skip to content

Commit 76917c6

Browse files
Shira SassoonCopilot
andcommitted
test: address review feedback on test quality (round 2)
- Fixture: clear FAB_TENANT_ID, FAB_SPN_*, FAB_MANAGED_IDENTITY env vars; reset _msal_app - Explicit-tenant test: assert subprocess.run not called (proves discovery bypassed) - Tenant-drift test: assert AzureCliCredential not instantiated (blocked before credential) - Fix expired-cache docstring: 'credential token request' not 'subprocess call' - Move misplaced test_error_status_code_on_credential_unavailable to TestAzureCliTokenAcquisition - Remove unused mock_run from status code test - Fix excess blank lines after fixture Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent e91c94b commit 76917c6

1 file changed

Lines changed: 35 additions & 26 deletions

File tree

tests/test_core/test_fab_auth_azure_cli.py

Lines changed: 35 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -20,24 +20,31 @@ def temp_dir_fixture(monkeypatch, tmp_path):
2020
monkeypatch.setattr(
2121
"fabric_cli.core.fab_state_config.config_location", lambda: str(tmp_path)
2222
)
23-
# Clear env vars that would interfere
24-
monkeypatch.delenv("FAB_TOKEN", raising=False)
25-
monkeypatch.delenv("FAB_TOKEN_ONELAKE", raising=False)
26-
monkeypatch.delenv("FAB_TOKEN_AZURE", raising=False)
23+
# Clear env vars that would interfere with auth
24+
for var in (
25+
"FAB_TOKEN",
26+
"FAB_TOKEN_ONELAKE",
27+
"FAB_TOKEN_AZURE",
28+
"FAB_TENANT_ID",
29+
"FAB_SPN_CLIENT_ID",
30+
"FAB_SPN_CLIENT_SECRET",
31+
"FAB_SPN_CERT_PATH",
32+
"FAB_MANAGED_IDENTITY",
33+
):
34+
monkeypatch.delenv(var, raising=False)
2735
# Clear singleton caches between tests
2836
auth = FabAuth()
2937
auth._azure_cli_token_cache.clear()
3038
auth._cached_az_tenant = None
3139
auth._cached_az_tenant_time = 0.0
3240
auth._auth_info = {}
41+
auth._msal_app = None
3342
# Update file paths to use the test's tmp_path
3443
auth.auth_file = os.path.join(str(tmp_path), "auth.json")
3544
auth.cache_file = os.path.join(str(tmp_path), "cache.bin")
3645
return str(tmp_path)
3746

3847

39-
40-
4148
class TestAzureCliIdentityType:
4249
"""Test that azure_cli is a valid identity type."""
4350

@@ -90,6 +97,7 @@ def test_set_azure_cli_explicit_tenant_overrides_auto(
9097
auth.set_access_mode("azure_cli")
9198
auth.set_azure_cli(tenant_id="explicit-tenant")
9299
assert auth.get_tenant_id() == "explicit-tenant"
100+
mock_run.assert_not_called()
93101

94102

95103
class TestAzureCliTokenAcquisition:
@@ -228,6 +236,25 @@ def test_unknown_exception_returns_safe_message(
228236
assert "eyJ0eXAi" not in str(exc_info.value)
229237
assert "manually to diagnose" in str(exc_info.value)
230238

239+
@patch("fabric_cli.core.fab_auth.AzureCliCredential")
240+
def test_error_status_code_on_credential_unavailable(
241+
self, mock_credential_class, temp_dir_fixture
242+
):
243+
"""CredentialUnavailableError should produce correct status code."""
244+
from fabric_cli.core.fab_auth import CredentialUnavailableError
245+
246+
auth = FabAuth()
247+
auth.set_access_mode("azure_cli")
248+
auth._azure_cli_token_cache.clear()
249+
250+
mock_instance = MagicMock()
251+
mock_instance.get_token.side_effect = CredentialUnavailableError("nope")
252+
mock_credential_class.return_value = mock_instance
253+
254+
with pytest.raises(FabricCLIError) as exc_info:
255+
auth._acquire_token_from_azure_cli(con.SCOPE_FABRIC_DEFAULT)
256+
assert exc_info.value.status_code == con.ERROR_AUTHENTICATION_FAILED
257+
231258

232259
class TestAzureCliTenantDrift:
233260
"""Test tenant drift detection during token acquisition."""
@@ -251,6 +278,7 @@ def test_tenant_drift_blocks_token_acquisition(
251278
auth._acquire_token_from_azure_cli(con.SCOPE_FABRIC_DEFAULT)
252279

253280
assert ErrorMessages.Auth.azure_cli_tenant_mismatch("original-tenant", "different-tenant") in str(exc_info.value)
281+
mock_credential_class.assert_not_called()
254282

255283
@patch("fabric_cli.core.fab_auth.AzureCliCredential")
256284
@patch("subprocess.run")
@@ -310,7 +338,7 @@ def test_cached_token_avoids_repeated_credential_calls(
310338
def test_expired_cache_triggers_refresh(
311339
self, mock_credential_class, temp_dir_fixture
312340
):
313-
"""Expired cached token should trigger a new subprocess call."""
341+
"""Expired cached token should trigger a new credential token request."""
314342
mock_token = MagicMock()
315343
mock_token.token = "fresh-token"
316344
mock_token.expires_on = int(time.time()) + 3600
@@ -559,22 +587,3 @@ def test_cache_miss_after_ttl_expiry(self, mock_run, temp_dir_fixture):
559587
result = auth._get_azure_cli_tenant()
560588
assert result == "new-tenant"
561589
mock_run.assert_called_once()
562-
563-
@patch("subprocess.run")
564-
def test_error_status_code_on_credential_unavailable(
565-
self, mock_run, temp_dir_fixture
566-
):
567-
"""CredentialUnavailableError should produce correct status code."""
568-
from fabric_cli.core.fab_auth import CredentialUnavailableError
569-
570-
auth = FabAuth()
571-
auth.set_access_mode("azure_cli")
572-
auth._azure_cli_token_cache.clear()
573-
574-
with patch("fabric_cli.core.fab_auth.AzureCliCredential") as mock_cred:
575-
mock_instance = MagicMock()
576-
mock_instance.get_token.side_effect = CredentialUnavailableError("nope")
577-
mock_cred.return_value = mock_instance
578-
with pytest.raises(FabricCLIError) as exc_info:
579-
auth._acquire_token_from_azure_cli(con.SCOPE_FABRIC_DEFAULT)
580-
assert exc_info.value.status_code == con.ERROR_AUTHENTICATION_FAILED

0 commit comments

Comments
 (0)