test(e2e): Migrate entity, file, secret, and studio tests from Platform-Deploy - #158
Conversation
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
|
mckornfield
left a comment
There was a problem hiding this comment.
should we have a companion PR to delete them from the other repo?
📝 WalkthroughWalkthroughAdds four independent E2E test modules covering the entities, files, secrets, and studio services. Each module provides pytest-based integration test coverage exercising CRUD operations, filtering, sorting, data validation, and API contract verification against a deployed NMP platform instance via SDK fixtures. ChangesE2E Test Suite Expansion
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
e2e/test_studio.py (1)
111-114: ⚡ Quick winWeak "production" assertion.
"production"appears in countless minified bundles (e.g.NODE_ENVguards) regardless ofVITE_APP_ENV. This can pass even when the env isn't baked, giving false confidence. Assert against the resolvedSTUDIO_UI_VITE_APP_ENVmarker value or a more specific build artifact.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/test_studio.py` around lines 111 - 114, The test's assertion using a generic "production" string is too weak; update the check in e2e/test_studio.py to assert the concrete baked-in marker/value instead of the word "production": read the expected STUDIO_UI_VITE_APP_ENV (or the known marker used in the UI build) and assert that exact marker/value appears in all_js_content (use the test variable all_js_content and the STUDIO_UI_VITE_APP_ENV identifier), or alternatively assert for a more specific build artifact string that only appears when VITE_APP_ENV was baked in.e2e/test_secrets.py (2)
20-40: ⚡ Quick winTests leak secrets into shared workspace; add cleanup.
Most tests create secrets and never delete them. Over time this pollutes the environment and increases flakiness (especially with paginated list assertions). Add cleanup (
try/finallyor fixture teardown) for each created secret.Also applies to: 49-70, 111-129, 137-157, 165-185
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/test_secrets.py` around lines 20 - 40, The test creates secrets (variables: secret_name, secret, retrieved_secret, list_response) but never deletes them; wrap each creation/verification block in a try/finally (or use a teardown fixture) and call the SDK delete method (e.g., sdk.secrets.delete(secret_name, workspace=workspace)) in the finally to ensure the secret is removed even on assertion failure; apply the same cleanup pattern to the other secret-creating blocks mentioned (lines 49-70, 111-129, 137-157, 165-185) so tests do not leak secrets into the shared workspace.
60-69: ⚡ Quick winUse
pytest.raiseswith a specific exception, notexcept Exception.Catching
Exceptioncan mask unrelated failures. Assert the expected error type/status and message viapytest.raises(...)so this test only passes on the duplicate-name path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/test_secrets.py` around lines 60 - 69, Replace the broad try/except with pytest.raises to assert the specific duplicate-secret error: wrap the sdk.secrets.create(...) call in pytest.raises(<ExpectedDuplicateException>) as excinfo (or use the SDK's duplicate error type such as sdk.exceptions.DuplicateResourceError or check for an HTTP 409 if the SDK surfaces requests.HTTPError), then assert "already exists" or "duplicate" in str(excinfo.value) (or assert excinfo.value.response.status_code == 409) to ensure the test only passes for the duplicate-name path; specifically update the block around sdk.secrets.create to use pytest.raises instead of catching Exception.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/test_entities.py`:
- Line 414: The test's assertion always passes because the expression uses "or
entity.name" which is truthy; update the assertion in the test (the line using
ENTITY_TYPE and entity.name) to remove the "or entity.name" fallback and assert
the actual pattern match directly (e.g., ensure
ENTITY_TYPE.replace("_","-").replace("-","") is contained in
entity.name.replace("-","")), and add a clear failure message referencing
ENTITY_TYPE and entity.name to make failures informative.
In `@e2e/test_secrets.py`:
- Around line 33-35: The list-based existence checks using sdk.secrets.list
(e.g., the block that builds secret_names = [s.name for s in list_response.data]
and asserts secret_name in secret_names) are flaky due to API pagination;
replace those assertions with sdk.secrets.retrieve(name=secret_name,
workspace=workspace) to confirm the secret exists (or call list with explicit
pagination/filter params that guarantee inclusion), and update the same pattern
at the other two locations referenced (around the blocks that use secret_name
and workspace on lines 89-92 and 175-180) so tests use retrieve or deterministic
paging instead of assuming all secrets are on page 1.
In `@e2e/test_studio.py`:
- Around line 130-135: The test currently silently passes when css_match is
None; update test_studio.py to explicitly handle the missing CSS link by adding
a check on css_match and calling pytest.skip("No CSS asset link found in
index_response") (or assert css_match is not None with a clear message) before
attempting to use css_path and sdk._client.get; reference the css_match variable
and the css_path/code that performs css_response = sdk._client.get(css_path) so
the test either skips or fails visibly when the CSS href is absent.
---
Nitpick comments:
In `@e2e/test_secrets.py`:
- Around line 20-40: The test creates secrets (variables: secret_name, secret,
retrieved_secret, list_response) but never deletes them; wrap each
creation/verification block in a try/finally (or use a teardown fixture) and
call the SDK delete method (e.g., sdk.secrets.delete(secret_name,
workspace=workspace)) in the finally to ensure the secret is removed even on
assertion failure; apply the same cleanup pattern to the other secret-creating
blocks mentioned (lines 49-70, 111-129, 137-157, 165-185) so tests do not leak
secrets into the shared workspace.
- Around line 60-69: Replace the broad try/except with pytest.raises to assert
the specific duplicate-secret error: wrap the sdk.secrets.create(...) call in
pytest.raises(<ExpectedDuplicateException>) as excinfo (or use the SDK's
duplicate error type such as sdk.exceptions.DuplicateResourceError or check for
an HTTP 409 if the SDK surfaces requests.HTTPError), then assert "already
exists" or "duplicate" in str(excinfo.value) (or assert
excinfo.value.response.status_code == 409) to ensure the test only passes for
the duplicate-name path; specifically update the block around sdk.secrets.create
to use pytest.raises instead of catching Exception.
In `@e2e/test_studio.py`:
- Around line 111-114: The test's assertion using a generic "production" string
is too weak; update the check in e2e/test_studio.py to assert the concrete
baked-in marker/value instead of the word "production": read the expected
STUDIO_UI_VITE_APP_ENV (or the known marker used in the UI build) and assert
that exact marker/value appears in all_js_content (use the test variable
all_js_content and the STUDIO_UI_VITE_APP_ENV identifier), or alternatively
assert for a more specific build artifact string that only appears when
VITE_APP_ENV was baked in.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b8a1bef4-4326-4172-a6e9-2485863c966e
📒 Files selected for processing (4)
e2e/test_entities.pye2e/test_files.pye2e/test_secrets.pye2e/test_studio.py
Context
This PR is Phase 1 of the e2e testing strategy (
docs/plans/e2e-test-strategy.md), seen here https://linear.app/nvidia/document/e2e-testing-strategy-7a9368744746E2E tests were temporarily moved to Platform-Deploy during OSS prep when internal Docker build infrastructure was separated out. Now that
nemo services runis the primary development and testing path, we're rebuilding e2e coverage against the subprocess backend. This PR ports the subset of Platform-Deploy's e2e tests that can run without Docker, Kubernetes, auth, or GPU — covering the core services whose interfaces haven't changed.What's in this PR
23 new tests ported from Platform-Deploy, added to the harness established in PR #125:
e2e/test_entities.py(8 tests) — Entity CRUD lifecycle, project association, listing with sorting/filtering, rename, auto-generated names, cluster-info endpointe2e/test_files.py(5 tests) — File upload/download, nested paths, cache status, file deletion, directory upload/downloade2e/test_secrets.py(6 tests) — Secret create/list/delete, duplicate name detection, secret value not exposed in create/retrieve/list responsese2e/test_studio.py(4 tests) — Studio index.html serving, SPA routing, JS bundle validation, CSS asset serving (self-skip when Studio UI is not built)Combined with the 4 existing smoke tests, the e2e suite is now 27 tests passing in ~14s.
Key adaptation
The nemo-platform SDK uses
sdk.secrets.create(value=...)while Platform-Deploy useddata=. All secret test calls updated accordingly.What's next (from the strategy doc)
This PR covers Phase 1 (subprocess core services). Subsequent phases:
services/→plugins/migration (evaluator, guardrails, auditor, etc.), write new tests against current APIs using Platform-Deploy as a reference for what to test, not a mechanical port.See
docs/plans/e2e-test-strategy.mdfor the full strategy, including backend abstraction, feature filtering, and CI integration plans.Test plan
make test-e2epasses (27 tests, ~14s)uv run pytest e2e -vwithout--run-e2eskips all e2e testsuv run pytest -v(full suite) does not run e2e testspython-e2e-testjob passesTracked by AIRCORE-712
Summary by CodeRabbit