Open mgorny opened 2 weeks ago
woodruffw
commented
2 weeks ago Yep, looks like we monkeypatch open to induce some failures, so that we get 100% coverage. I suppose we could switch detect_gcp to use either a wrapper around open or a higher level API (like Path), which presumably wouldn't be as much of a patching issue for PyPy?
mgorny
commented
2 weeks ago My gut feeling is that there must be a portable way to mock it. I'll experiment with it today.
mgorny
commented
2 weeks ago FWICS, you can patch it using unittest.mock directly, but for some reason pytest doesn't seem to wrap the plain patch call, e.g.:
diff --git a/test/unit/internal/oidc/test_ambient.py b/test/unit/internal/oidc/test_ambient.py
index 0d5d589..0c532c6 100644
--- a/test/unit/internal/oidc/test_ambient.py
+++ b/test/unit/internal/oidc/test_ambient.py
@@ -16,6 +16,7 @@ import json
import pretend
import pytest
+from unittest.mock import patch
from requests import HTTPError, Timeout
from id import detect_credential
@@ -402,17 +403,18 @@ def test_gcp_impersonation_succeeds(monkeypatch):
def test_gcp_bad_env(monkeypatch):
oserror = pretend.raiser(OSError)
- monkeypatch.setitem(ambient.__builtins__, "open", oserror) # type: ignore
+ #monkeypatch.setitem(ambient.__builtins__, "open", oserror) # type: ignore
logger = pretend.stub(debug=pretend.call_recorder(lambda s: None))
monkeypatch.setattr(ambient, "logger", logger)
- assert ambient.detect_gcp("some-audience") is None
- assert logger.debug.calls == [
- pretend.call("GCP: looking for OIDC credentials"),
- pretend.call("GCP: GOOGLE_SERVICE_ACCOUNT_NAME not set; skipping impersonation"),
- pretend.call("GCP: environment doesn't have GCP product name file; giving up"),
- ]
+ with patch("id._internal.oidc.ambient.open", oserror):
+ assert ambient.detect_gcp("some-audience") is None
+ assert logger.debug.calls == [
+ pretend.call("GCP: looking for OIDC credentials"),
+ pretend.call("GCP: GOOGLE_SERVICE_ACCOUNT_NAME not set; skipping impersonation"),I can make a (cleaner) pull request if you wish. That said, pytest docs advise against patching open() as it can apparently conflict with pytest internals.
woodruffw
commented
2 weeks ago Thanks for looking into it. Yeah, I think we should probably revisit our use of open(...) rather than risk future issues with pytest. I'll look at a separate PR for that today.
The few gcp-related tests seem to be failing. I think they're trying to mock CPython implementation details:
Reproduced on top of 8dfaa83e8d21ebb97138a2d43e1b95260817f2b3.