Skip to content

test(admin): adopt echotest and testify in admin, auditlog, and cache tests - #982

Open
SantiagoDePolonia wants to merge 3 commits into
test/shared-test-helpersfrom
test/testify-admin
Open

test(admin): adopt echotest and testify in admin, auditlog, and cache tests#982
SantiagoDePolonia wants to merge 3 commits into
test/shared-test-helpersfrom
test/testify-admin

Conversation

@SantiagoDePolonia

Copy link
Copy Markdown
Contributor

Rewrites the internal/admin, internal/auditlog, internal/responsecache, and internal/telemetry tests on top of the shared echotest helper and testify.

  • Hand-built echo request/recorder/context blocks and the per-file newHandlerContext and decode helpers become echotest.Get/Post/Request and echotest.Decode; echo.New() remains only where the real router is exercised.
  • Hand-rolled assertions converted to require/assert; pointer identity checks use NotSame.
  • Redundant tests removed or merged: constructor non-nil checks and a subset test in admin/dashboard, TestParseUsageParams_IntervalEmpty (identical to an assertion in TestParseUsageParams_DaysDefault), the two recalculate-confirmation tests and the slash-in-name credential test folded into tables, TestNoopLogger (does-not-panic), TestSQLStore_WriteBatch_Chunking (covered by the boundary and parameter-limit tests), and the two identical cache-control skip tests merged into one.

Test-only change, about 5,800 net lines removed. Stacked on #976 (helper packages).

@coderabbitai

coderabbitai Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

Next included review available in 11 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: b544df29-7339-47c4-9d24-5f4c587facf2

📥 Commits

Reviewing files that changed from the base of the PR and between 958e420 and c46cd51.

📒 Files selected for processing (64)
  • internal/admin/audit_projection_test.go
  • internal/admin/dashboard/dashboard_test.go
  • internal/admin/dashboard_config_contract_test.go
  • internal/admin/handler_audit_sessions_test.go
  • internal/admin/handler_audit_stats_test.go
  • internal/admin/handler_authkeys_test.go
  • internal/admin/handler_budgets_test.go
  • internal/admin/handler_guardrails_test.go
  • internal/admin/handler_live_test.go
  • internal/admin/handler_mcpservers_test.go
  • internal/admin/handler_model_pricing_overrides_test.go
  • internal/admin/handler_models_test.go
  • internal/admin/handler_plugins_test.go
  • internal/admin/handler_pricing_test.go
  • internal/admin/handler_provider_credentials_test.go
  • internal/admin/handler_providers_health_test.go
  • internal/admin/handler_providers_test.go
  • internal/admin/handler_ratelimits_test.go
  • internal/admin/handler_runtime_settings_test.go
  • internal/admin/handler_scope_test.go
  • internal/admin/handler_tagging_test.go
  • internal/admin/handler_test.go
  • internal/admin/handler_users_test.go
  • internal/admin/handler_virtualmodels_plugin_test.go
  • internal/admin/handler_virtualmodels_test.go
  • internal/admin/handler_workflows_test.go
  • internal/admin/routes_test.go
  • internal/auditlog/attempt_capture_test.go
  • internal/auditlog/audio_body_test.go
  • internal/auditlog/auditlog_test.go
  • internal/auditlog/authentication_events_test.go
  • internal/auditlog/body_test.go
  • internal/auditlog/conversation_helpers_test.go
  • internal/auditlog/enrich_test.go
  • internal/auditlog/entry_capture_test.go
  • internal/auditlog/guardrail_outcomes_test.go
  • internal/auditlog/image_body_test.go
  • internal/auditlog/middleware_auth_method_test.go
  • internal/auditlog/middleware_test.go
  • internal/auditlog/pending_revisions_test.go
  • internal/auditlog/reader_mongodb_test.go
  • internal/auditlog/reader_sessions_mongodb_test.go
  • internal/auditlog/reader_sql_boundary_test.go
  • internal/auditlog/roundtrip_sql_test.go
  • internal/auditlog/search_index_test.go
  • internal/auditlog/session_id_test.go
  • internal/auditlog/stats_test.go
  • internal/auditlog/store_mongodb_test.go
  • internal/auditlog/store_sql_test.go
  • internal/auditlog/stream_entry_request_fields_test.go
  • internal/auditlog/stream_observer_test.go
  • internal/auditlog/timestamp_utc_test.go
  • internal/auditlog/user_path_filter_test.go
  • internal/responsecache/exact_cache_test.go
  • internal/responsecache/handle_request_test.go
  • internal/responsecache/plugin_nostore_test.go
  • internal/responsecache/readiness_test.go
  • internal/responsecache/semantic_extract_test.go
  • internal/responsecache/semantic_test.go
  • internal/responsecache/sse_validation_test.go
  • internal/telemetry/environment_test.go
  • internal/telemetry/observer_test.go
  • internal/telemetry/telemetry_test.go
  • internal/telemetry/yaml_test.go

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Sep 12, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

Safe to merge: the remaining concern affects failure diagnostics in tests and does not change production behavior.

Reviews (1) · Last reviewed commit: "test(admin): adopt echotest and testify ..."

Comment thread internal/admin/handler_test.go Outdated
Comment on lines +490 to +491
assert.NotNil(t, models[0].TotalCost)
assert.Equal(t, 1.5, *models[0].TotalCost)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Require pointers before dereferencing

assert.NotNil records a failure but allows this test to continue, so a nil decoded TotalCost then panics at the following dereference instead of reporting the useful contract failure. Use require.NotNil before this dereference and the equivalent checks at lines 547 and 587. This is non-blocking, but it makes response-decoding regressions harder to diagnose.

Artifacts

Evidence from the check

  • A temporary Go test defines the assert-and-dereference and require-and-dereference nil-pointer patterns used for the runtime comparison; it directly reproduces the relevant control flow.

Command output from the check

  • Two executed `go test` runs show that the assert pattern reports nil then panics, while the require pattern reports nil and terminates without a panic; require preserves the intended diagnostic.

View artifacts

T-Rex Ran code and verified through T-Rex

@SantiagoDePolonia

Copy link
Copy Markdown
Contributor Author

The three assert.NotNil before dereference sites (lines 490, 547, 587) are switched to require.NotNil in e9c353a, together with the other assert-before-dereference sites in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant