feat(e2e): comprehensive live-smoke coverage across all captured verbs#18
Merged
Conversation
Adds e2e/connector_snowflake_test.go covering password, keypair, oauth-sso, and oauth-individual leaves. Each test skips when its required ANA_E2E_SF_* env vars are unset (no defaults — Snowflake auth shapes make unreachable defaults meaningless). Harness additions: - RunStdin(stdin, args...) — plumbs stdin into the existing Run path so tests can exercise --*-stdin secret flags without bypassing the CLI layer. - RegisterConnectorCleanup(id) — LIFO cleanup for connectors created via CLI dispatch (id parsed from stdout) rather than the raw-RPC helper. - client.go wires ANA_E2E_ENDPOINT into connector.Deps.Endpoint so the oauth-sso success note renders against the active endpoint.
- list + list --json assert header and JSON shape - folders list + --json - get + --json discover an id from list (skip if org has no dashboards) - health + spawn env-gated on ANA_E2E_DASHBOARD_ID since spawn touches billed runtime quotas
- list + list --json header + JSON shape
- get + --json discover an id from list --json (skip if org is empty)
- reports default assertion is header-only — zero historical runs still
emits the RUN_ID/SUBJECT/RAN_AT row
- lineage accepts either the FROM/TO/TYPE table or the "(no lineage edges)"
marker; catalog sample is literally `{}` so either is contract-valid
- list + list --json header + JSON shape - get + --json discover an integer id from list --json (skip if org has none); command rejects non-integer ids at dispatch, so we always pass the captured int
- show + show --json header + JSON shape (`posts`) - stats renders multi-key summary; JSON mode asserts at least one of the catalog counters is present (values drift every tick)
- --json asserts JSON Lines (each line round-trips through encoding/json) - no-since covers the omitempty branch where `since` drops off the wire - default render now asserts all four header columns
- org roles list + --json (RBACService/ListRoles) - org permissions list + --json (RBACService/ListPermissions) - org show/list/members list each gain a --json shape check (organization, organizations, members respectively)
- full round-trip through harness temp XDG using --token-stdin on add - list/show --json assert hasToken + active flags - error-path smokes: missing <name> on add, unknown profile on use
- keys create/rotate/revoke through h.Run — id recovered from `list --json` since the create verb only emits the plaintext token - service-accounts create/delete parses the `<memberId> <name>` line - list --json for keys + service-accounts asserts the envelope key - error-path smokes: missing --name on keys create, extra positional on keys rotate
…les/test smokes - list/get --json envelope assertions - postgres password leaf via --password-stdin with --ssl=false and --ssl=true - update --password-stdin + rename multi-flag merge - tables (env-gated on ANA_E2E_PG_HOST), examples, test (OK|FAIL:) leaves - get <missing-id> error-path smoke
…ookmark - list/get --json envelope assertions (chats array, chat object id round-trip) - chat new via h.Run with CLI-path delete cleanup and list round-trip - history smoke (endpoint answers without error for a fresh chat) - bookmark/unbookmark chain printing `ok` - chat show <missing uuid> error-path smoke
- Expand optional env table with ANA_E2E_DASHBOARD_ID (dashboard spawn/health gate) - Add a dedicated Postgres connector env section matching the Snowflake one - Spell out defaults (e2e.invalid / 5432 / e2e / postgres) and which vars gate `connector tables` (requires a reachable host)
Ephemeral is the correct pattern: the test should create its own dashboard and register a deferred delete. Blocked on two missing pieces: - `chat new --dashboard-mode` flag (CLI doesn't expose the dashboardMode wire field today, even though the struct carries it) - DashboardService delete endpoint (not in api-catalog yet — webapp must have it, but the path isn't known; can't cascade-clean without it) Until both land the tests stay env-gated on ANA_E2E_DASHBOARD_ID and skip when the var is unset.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds extensive e2e CLI tests across multiple subsystems, extends the e2e harness to support stdin and endpoint propagation to connector wiring, and adds by-name cleanup helpers and other test utilities. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/connector_test.go (1)
18-21:⚠️ Potential issue | 🟠 MajorReject malformed
ANA_E2E_PG_PORTvalues instead of silently using 5432.If the env var is set to a non-numeric value, these tests will target port
5432anyway. On a real host that can hit the wrong database and make operator misconfiguration hard to notice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/connector_test.go` around lines 18 - 21, The code currently calls strconv.Atoi(os.Getenv("ANA_E2E_PG_PORT")) and silently falls back to 5432 on parse errors; change the logic to detect whether ANA_E2E_PG_PORT is set (use os.LookupEnv or check empty string) and if it is set but strconv.Atoi (or strconv.ParseInt) returns an error, fail the test (t.Fatalf or log.Fatalf) with a clear message about the malformed port; if the env var is unset, keep the default port = 5432. Ensure you update the code paths that reference port so they rely on this validated value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/auth_test.go`:
- Around line 115-138: After successfully creating the API key with
h.Run("auth", "keys", "create", "--name", name) register a best-effort cleanup
immediately (use h.Register) before performing lookups/assertions so the
mutation is ledgered; then, once apiKeyIDByName(t, h, name) returns the final
id, replace or tighten that registration to revoke by id (h.Run("auth", "keys",
"revoke", id)) and fall back to recording a manual revert if revoke fails. In
short: schedule a provisional h.Register-based cleanup right after the create
mutation succeeds (referencing name or a placeholder), and update it to revoke
by id after apiKeyIDByName yields the concrete id, ensuring LIFO revert and
manual-revert.md reporting on failures.
In `@e2e/chat_test.go`:
- Around line 183-218: The registered cleanup closure in TestChatNewCLI captures
the second return value from h.Run as delErr but that value is actually stderr
(a string); rename delErr to stderr (or stderrDel) in the closure to avoid
confusion and update the fmt.Sprintf call inside h.RecordManualRevert to use the
renamed variable (references: TestChatNewCLI, h.Register, h.Run,
h.RecordManualRevert).
In `@e2e/connector_snowflake_test.go`:
- Around line 102-110: The test currently calls extractConnectorID and then
h.RegisterConnectorCleanup after creating a Snowflake connector, which leaves an
orphaned connector if stdout changes and parsing fails; change this by adding a
helper (e.g., registerConnectorCleanupFromCreate) that accepts the raw
stdout/stderr and the command args, calls h.RegisterConnectorCleanup immediately
with a best-effort id (or a sentinel entry) and attempts to parse the ID via
extractConnectorID; if parsing succeeds, replace/update the sentinel with the
real ID, and if parsing fails, record the raw stdout/stderr and the attempted
create command into a manual-revert.md report so operators can follow up; update
all create sites that call h.RunStdin + extractConnectorID +
h.RegisterConnectorCleanup (including the other leaves around lines 142-150,
172-180, 209-217) to use the new helper and ensure cleanup is always ledgered in
LIFO order.
In `@e2e/connector_test.go`:
- Around line 155-163: The test currently calls h.RunStdin(...), then parses
stdout with extractConnectorID and only afterwards calls
h.RegisterConnectorCleanup, risking leaked connectors if parsing fails; change
this by adding or using a harness helper (e.g., CreateAndRegisterConnector or an
atomic wrapper around RunStdin) that runs RunStdin, extracts the connector id
internally using extractConnectorID, and immediately calls
RegisterConnectorCleanup before returning the id/stdout to the test; update the
two affected call sites (the block using RunStdin + extractConnectorID +
RegisterConnectorCleanup around lines shown and the similar block at 185-193) to
call this new atomic helper so cleanup is always registered even if stdout
parsing fails, and ensure the helper still returns errors and stdout for
assertions.
In `@e2e/dashboard_test.go`:
- Around line 89-95: The test currently treats an empty stdout as success by
returning early; instead, when stdout == "" the test should fail fast. In the
e2e/dashboard_test.go test that calls json.Unmarshal on stdout, replace the
early "return" for stdout == "" with a t.Fatalf (or t.Fatalf-like) call that
reports "folders list --json: empty stdout" so the JSON path is always exercised
and empty output is treated as a test failure; keep the existing json.Unmarshal
error handling (t.Fatalf("folders list --json: not valid JSON: %v\nstdout=%q",
err, stdout)) unchanged.
In `@e2e/harness/client.go`:
- Line 93: The code calls connector.New with os.Getenv("ANA_E2E_ENDPOINT")
instead of using the already-resolved endpoint from loadEnv/buildTransport;
update the call chain to accept and thread the resolved endpoint through
buildVerbs (or the function that constructs the transport) so connector.New
receives the pre-resolved endpoint value instead of re-reading the environment,
e.g., modify buildVerbs/Begin to accept an endpoint parameter and pass
env.endpoint into connector.New to avoid duplicate env lookups.
---
Outside diff comments:
In `@e2e/connector_test.go`:
- Around line 18-21: The code currently calls
strconv.Atoi(os.Getenv("ANA_E2E_PG_PORT")) and silently falls back to 5432 on
parse errors; change the logic to detect whether ANA_E2E_PG_PORT is set (use
os.LookupEnv or check empty string) and if it is set but strconv.Atoi (or
strconv.ParseInt) returns an error, fail the test (t.Fatalf or log.Fatalf) with
a clear message about the malformed port; if the env var is unset, keep the
default port = 5432. Ensure you update the code paths that reference port so
they rely on this validated value.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6daa2fb4-eeda-439c-8358-bc2f412d7252
⛔ Files ignored due to path filters (5)
docs/CLAUDE.mdis excluded by!**/docs/**,!**/*.mddocs/prompts/CLAUDE.mdis excluded by!**/docs/**,!**/*.mddocs/prompts/e2e-comprehensive-coverage.mdis excluded by!**/docs/**,!**/*.mde2e/CLAUDE.mdis excluded by!**/*.mde2e/README.mdis excluded by!**/*.md
📒 Files selected for processing (14)
e2e/audit_test.goe2e/auth_test.goe2e/chat_test.goe2e/connector_snowflake_test.goe2e/connector_test.goe2e/dashboard_test.goe2e/feed_test.goe2e/harness/client.goe2e/harness/harness.goe2e/harness/resources.goe2e/ontology_test.goe2e/org_test.goe2e/playbook_test.goe2e/profile_test.go
3 tasks
Address PR #18 review: - harness/resources.go: add RegisterConnectorCleanupByName, RegisterAPIKeyCleanupByName, RegisterServiceAccountCleanupByName so callers can pre-register a name-based safety net before mutating. LIFO ordering makes the by-name cleanup a no-op when the id-based cleanup that runs after a successful parse already covered it. - auth/connector/snowflake CLI tests: pre-register name-based cleanup before invoking the CLI so a failed id-extraction can't orphan a resource server-side. - harness/client.go + harness.go: thread resolved endpoint into buildVerbs so the connector verb sees the same value the harness already resolved (was reading os.Getenv twice). - chat_test.go: rename stderr capture var to delStderr for accuracy. - dashboard_test.go: fail fast on empty --json stdout instead of silently returning.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
e2e/chat_test.go (1)
195-213:⚠️ Potential issue | 🟠 MajorRegister a fallback cleanup before
chat newoutput parsing.
chat newmutates server state beforechatIDis parsed. If parsing fails,t.Fatalfaborts before any cleanup is registered, which can orphan the chat.As per coding guidelines, “Every mutation in e2e tests must be ledgered and reverted in LIFO order at test end.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/chat_test.go` around lines 195 - 213, The test registers cleanup only after parsing chatID, so if parsing fails the created chat can be orphaned; change the sequence so that immediately after h.Run returns successfully (err == nil) you call h.Register to add a cleanup closure that captures the raw stdout and performs the delete (inside the closure compute id := strings.TrimSpace(stdout) and if id != "" run "chat delete" and RecordManualRevert on error); keep the existing t.Fatalf checks for err and for empty id but ensure h.Register is invoked right after confirming err == nil (before any parsing or t.Fatalf) using the same variables (stdout, stderr) so cleanup will run even if parsing or subsequent checks fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/harness/resources.go`:
- Around line 91-97: When the by-name list call (h.client.Unary with listPath)
fails in the cleanup-by-name block, record a manual-revert entry instead of
silently returning: write an entry to the manual-revert.md report describing the
unreverted mutation (include the connector name, listPath, and the error string)
so operators can follow up, then continue returning as before; apply the same
change to the other two occurrences around the cleanup logic (the blocks
referencing h.client.Unary, listPath, h.t.Logf at the other locations).
---
Duplicate comments:
In `@e2e/chat_test.go`:
- Around line 195-213: The test registers cleanup only after parsing chatID, so
if parsing fails the created chat can be orphaned; change the sequence so that
immediately after h.Run returns successfully (err == nil) you call h.Register to
add a cleanup closure that captures the raw stdout and performs the delete
(inside the closure compute id := strings.TrimSpace(stdout) and if id != "" run
"chat delete" and RecordManualRevert on error); keep the existing t.Fatalf
checks for err and for empty id but ensure h.Register is invoked right after
confirming err == nil (before any parsing or t.Fatalf) using the same variables
(stdout, stderr) so cleanup will run even if parsing or subsequent checks fail.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 10a9b716-5474-4b23-a573-e1b0b4e1d1f0
⛔ Files ignored due to path filters (1)
e2e/harness/CLAUDE.mdis excluded by!**/*.md
📒 Files selected for processing (8)
e2e/auth_test.goe2e/chat_test.goe2e/connector_snowflake_test.goe2e/connector_test.goe2e/dashboard_test.goe2e/harness/client.goe2e/harness/harness.goe2e/harness/resources.go
When the by-name cleanup's list call fails, record a manual-revert entry so any orphan resource is visible to operators. Previously the failure was logged and swallowed, which violates the harness contract that anything unreverted lands in manual-revert.md. Addresses PR #18 review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/harness/resources.go`:
- Around line 107-112: The delete-by-name cleanup currently treats any error
from h.client.Unary as a failure and records a manual revert; update the block
that calls h.client.Unary (the DeleteConnector call) to treat a 404/not-found as
non-error: after the call, if err != nil check whether the error represents an
HTTP 404 (e.g., via errors.As to the API/error type your client returns or by
inspecting an HTTP status field); if it's a not-found, simply ignore it (no
h.t.Errorf and do not call h.RecordManualRevert), otherwise keep the existing
h.t.Errorf and h.RecordManualRevert behavior. Ensure you reference the same
symbols: h.client.Unary, DeleteConnector (delPath), and h.RecordManualRevert
when implementing the conditional.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 955d0cd3-1791-4cad-823d-9ed943d09fc6
📒 Files selected for processing (1)
e2e/harness/resources.go
Match the API-key and service-account by-name variants: a 404 on DeleteConnector means the id-based cleanup (which runs first LIFO) already got the row, which is the happy path. Was tripping false manual-revert entries for every successful CLI create. Addresses PR #18 review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/harness/resources.go`:
- Around line 66-68: The exported cleanup registration helpers (e.g.,
RegisterConnectorCleanup) currently register callbacks that perform real RPCs at
End(), violating dry-run mode; update each exported helper to first check
h.env.dryRun and return without registering the callback (or register a no-op)
when dryRun is true, and also defensively validate the provided id (e.g., id >
0) before calling h.Register to avoid scheduling invalid deletions; locate
RegisterConnectorCleanup and the other exported cleanup helpers that add
h.Register(func() { ... }) and apply this guard there.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2e030385-efee-469a-8719-451e207d3614
📒 Files selected for processing (1)
e2e/harness/resources.go
…id ids
Register{Connector,APIKey,ServiceAccount}CleanupByName and
RegisterConnectorCleanup now short-circuit when ANA_E2E_DRYRUN=1
(or when the argument is zero/empty), so the helpers can never
schedule a real RPC from End() when no mutation actually happened.
This was referenced Apr 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
h.Run/h.RunStdin/h.RunJSON, asserting--jsonenvelopes and error-path smokes for every surface.ANA_E2E_DASHBOARD_IDwith aTODOpointing at ephemeral creation; blocked onchat new --dashboard-mode(not exposed) and DashboardService delete endpoint (not yet in api-catalog).e2e/README.mdgets a Postgres env section + dashboard gate;docs/prompts/captures the coverage brief that drove this sprint.Known pre-existing failures (NOT introduced here)
TestAuthKeysCreateCLI,TestAuthKeysRotateCLI,TestAuthServiceAccountsCreateCLI— server now requiresassumed_rolesorinherit_all_roles=true; CLI flag work tracked separately.TestChatNewCLI— list-by-title lookup occasionally can't find the freshly-created chat (list staleness).TestConnectorTest— CLIconnector test <id>is fundamentally at odds with the server endpoint (which wants a full config body, not an id). Fix drafted locally; parked for a follow-up PR.Test plan
make e2eagainst the demo org; all other tests green; Snowflake password / keypair / oauth-sso / oauth-individual all green.ANA_CLI_E2E_ROLE,ANA_CLI_E2E_USER,ANA_CLI_E2E_OAUTH) — see.envtemplate keys ine2e/README.md.Summary by CodeRabbit
Tests
Chores