Conversation
Add end-to-end tests that exercise the SDK against a real Descope backend (sync + async via a unified client wrapper), gated behind the `e2e` pytest marker and a dedicated CI job (with debounce + sandbox secrets). Unit CI runs with --ignore=tests/e2e so e2e stays opt-in.
|
🐕 Review complete — View session on Shuni Portal 🐾 |
🐕 Suggested ReviewersThe review assignment focuses on contributors who have made recent commits to critical configuration and workflow files, ensuring thorough coverage of CI integration and project setup for the new end-to-end testing suite.
Suggested by Shuni based on git history and PR context. Names are not @-mentioned to avoid notifying anyone — request a review from whoever fits best. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| assert jwt_response[REFRESH_SESSION_TOKEN_NAME]["jwt"], "Refresh token missing after sign-in" | ||
| assert jwt_response.get("firstSeen") is False | ||
| assert jwt_response.get("user", {}).get("name") == "Test User" | ||
| refresh_token = jwt_response[REFRESH_SESSION_TOKEN_NAME]["jwt"] |
| finally: | ||
| try: | ||
| await descope_client.invoke(descope_client.mgmt.user.delete(login_id)) | ||
| except Exception: |
| finally: | ||
| try: | ||
| await descope_client.invoke(descope_client.mgmt.user.delete(login_id)) | ||
| except Exception: |
| finally: | ||
| try: | ||
| await descope_client.invoke(descope_client.mgmt.user.delete(login_id)) | ||
| except Exception: |
| for rname in [role1_name, role2_name]: | ||
| try: | ||
| await descope_client.invoke(descope_client.mgmt.role.delete(rname)) | ||
| except Exception: |
| for uid in [invited_login_id, invited1_login_id, invited2_login_id]: | ||
| try: | ||
| await descope_client.invoke(descope_client.mgmt.user.delete(uid)) | ||
| except Exception: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![shuni-bot[bot]](https://avatars.githubusercontent.com/in/2536750?s=60&v=4){kind=small}
There was a problem hiding this comment.
🐕 Shuni's Review
Adds an e2e test suite for auth flows + management APIs, with a CI e2e job and shared sync/async harness. Solid coverage and clean teardown — good bones!
Sniffed out 5 issues (test/CI only — no production impact):
- 2 🟡 MEDIUM:
skip_debounceCI input is dead code; e2econftestaborts the whole pytest session when env vars are missing - 3 🟢 LOW: substring
localhostTLS-skip check, inaccurate "shared base" docstring, fragile magic-link token parsing
See inline comments. Woof!
| with: | ||
| report_paths: e2e-report.xml | ||
| check_name: E2E Test Report | ||
|
|
There was a problem hiding this comment.
🟡 MEDIUM: The skip_debounce input can never actually skip the debounce.
This step is guarded by github.event_name == 'pull_request', but skip_debounce can only be set on a workflow_dispatch run — on which this step is already skipped. On real PR events inputs.skip_debounce is null, and null != 'true' is always true, so the debounce always runs regardless. The input is effectively dead. (Separately, line 13's description says "5-minute debounce" but sleep 60 is 1 minute.)
Drop the event guard and evaluate the input directly, e.g. !fromJSON(inputs.skip_debounce || 'false'), so dispatch runs can honor it.
| "ERROR: DESCOPE_PROJECT_ID and DESCOPE_MANAGEMENT_KEY must be set to run e2e tests", | ||
| file=sys.stderr, | ||
| ) | ||
| pytest.exit("Missing required e2e environment variables", returncode=1) |
There was a problem hiding this comment.
🟡 MEDIUM: Module-level pytest.exit() aborts the entire pytest session, not just the e2e tests.
tests/e2e/conftest.py is imported at collection time whenever pytest discovers tests/e2e/. CI is safe because the unit job passes --ignore=tests/e2e, but a developer running pytest tests/ locally without DESCOPE_PROJECT_ID/DESCOPE_MANAGEMENT_KEY gets a session-level SystemExit that cancels their unit tests too. Prefer skipping only the e2e tests (e.g. pytest.skip(..., allow_module_level=True) or a session fixture) so unmarked tests still run.
| project_id = os.environ["DESCOPE_PROJECT_ID"] | ||
| management_key = os.environ["DESCOPE_MANAGEMENT_KEY"] | ||
|
|
||
| skip_verify = "localhost" in os.environ.get("DESCOPE_BASE_URI", "") |
There was a problem hiding this comment.
🟢 LOW: "localhost" in DESCOPE_BASE_URI is a substring match, so a host like https://localhost.example.com would silently disable TLS verification against a real remote endpoint.
Low risk since the value comes from a controlled secret, but a hostname check is safer:
| skip_verify = "localhost" in os.environ.get("DESCOPE_BASE_URI", "") | |
| from urllib.parse import urlparse | |
| skip_verify = urlparse(os.environ.get("DESCOPE_BASE_URI", "")).hostname in {"localhost", "127.0.0.1", "::1"} |
| """ | ||
| Shared base class for unified sync/async client wrappers. | ||
|
|
||
| Both the unit-test UnifiedClient (tests/conftest.py) and the e2e wrapper |
There was a problem hiding this comment.
🟢 LOW: This docstring says both UnifiedClient (tests/conftest.py) and the e2e wrapper "inherit from this", but neither actually does — UnifiedClient is a standalone copy of the same three methods, and the e2e fixture instantiates UnifiedClientBase directly. The shared base isn't shared yet. Either make UnifiedClient subclass UnifiedClientBase (and drop the duplicated __init__/__getattr__/invoke) or correct the docstring.
| ) | ||
| ) | ||
| raw_link = td["link"] | ||
| token = raw_link[raw_link.index("=") + 1 :] |
There was a problem hiding this comment.
🟢 LOK: Token extraction via raw_link.index("=") grabs everything after the first = in the URL. It works for uri="http://test.me" today, but breaks if the link ever carries an earlier query param, and raises ValueError (unhandled) if no = is present. Same at line 73. Consider parsing the t param explicitly: parse_qs(urlparse(raw_link).query)["t"][0].
Avoids CodeQL py/incomplete-url-substring-sanitization on domain membership assertions and checks the full domain list.
Coverage reportThe coverage rate went from None of the new lines are part of the tested code. Therefore, there is no coverage data about them. |
2 participants
Related Issues
Fixes: https://github.com/descope/etc/issues/16390
In a Nutshell
Description
Adds an end-to-end test suite under
tests/e2ethat exercises authentication flows and management APIs against a live project. Includes shared fixtures/config (conftest.py,_unified.py), a CI job to run the suite, and the supportingpyproject.toml/uv.lockdependency updates.Must