Add unit tests for azure.ai.routines client and fix MarkHidden nit#8790
Conversation
Address follow-up items from issue #8365: - Add comprehensive httptest-based unit tests for the routines data-plane client (client_test.go): covers GetRoutine, ListRoutines (single/multi page), PutRoutine, DeleteRoutine, EnableRoutine, DisableRoutine, DispatchRoutineAsync, ListRoutineRuns (pagination, Top cap, filter), URL construction helpers, and context cancellation. - Add unit tests for boolStr and sortedKeys helpers (routine_helpers_test.go). - Add explanatory comment for MarkHidden error discard in routine_update.go (item #10: the flags are registered immediately above, so MarkHidden cannot fail). Closes #8365 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
trangevi
requested review from
JeffreyCA,
glharper,
huimiu,
hund030,
therealjohn and
trrwilson
as code owners
📋 Prioritization NoteThanks for the contribution! The linked issue isn't in the current milestone yet. |
There was a problem hiding this comment.
Pull request overview
This PR improves the azure.ai.routines extension’s reliability by adding unit test coverage for the data-plane routines.Client and by clarifying a previously-silent MarkHidden error discard in the update command.
Changes:
- Adds
httptest-based unit tests covering theroutines.ClientAPI surface (CRUD, pagination, actions, dispatch, run listing, URL helpers, context cancellation). - Adds unit tests for
boolStrandsortedKeyshelper functions. - Documents why ignoring
MarkHiddenerrors inroutine_updateis safe.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| cli/azd/extensions/azure.ai.routines/internal/pkg/routines/client_test.go | New unit tests for the routines data-plane client (CRUD/actions/pagination/helpers). |
| cli/azd/extensions/azure.ai.routines/internal/cmd/routine_update.go | Adds an explanatory comment for the intentional MarkHidden error discard. |
| cli/azd/extensions/azure.ai.routines/internal/cmd/routine_helpers_test.go | New unit tests for boolStr and sortedKeys. |
…ring - Replace t.Fatal() calls inside httptest handler goroutines with w.WriteHeader(500) to avoid panics from non-test goroutines. - Move w.Header().Set() calls before w.WriteHeader() so Content-Type headers are actually sent in responses. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Tests are thorough and well-structured: good use of t.Parallel(), t.Context(), atomic.Int32 for concurrency safety, and comprehensive coverage of happy paths, error responses, pagination, and URL encoding.
CI blocker: The golangci-lint job is red due to 22 gosec G104: Errors unhandled violations on w.Write(...) and json.NewEncoder(w).Encode(...) calls in test handlers. Quick fix: assign to blank identifier on each one, e.g. _ = json.NewEncoder(w).Encode(routine). Alternatively, a file-level //nolint:gosec directive at the top of the test file would suppress them all, though per-line discards are more explicit.
I concur with the existing Copilot feedback on require usage inside httptest handler goroutines (lines 165, 300). Switching those two to assert.NoError (or decoding into a local and checking after the request round-trips) avoids potential goroutine panics.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Two items blocking this PR:
gosec G104 (CI red): golangci-lint is failing because w.Write(...) and json.NewEncoder(w).Encode(...) return values aren't consumed in test handlers. The sibling tests in azure.ai.agents/optimize_api use _, _ = w.Write(...) and _ = json.NewEncoder(w).Encode(...). Apply that pattern to all 22 call sites.
require in httptest goroutines: The second commit replaced explicit t.Fatal() calls with w.WriteHeader(500), but two require.NoError calls remain inside handlers (lines 165, 300). require calls t.FailNow(), which panics from non-test goroutines. See inline suggestions for the fix.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Two items blocking this PR:
gosec G104 (CI red): golangci-lint is failing because w.Write(...) and json.NewEncoder(w).Encode(...) return values aren't consumed in test handlers. The sibling tests in azure.ai.agents/optimize_api use _, _ = w.Write(...) and _ = json.NewEncoder(w).Encode(...). Apply that pattern to all 22 call sites.
require in httptest goroutines: The second commit replaced explicit t.Fatal() calls with w.WriteHeader(500), but two require.NoError calls remain inside handlers (lines 165, 300). require calls t.FailNow(), which panics from non-test goroutines. See inline suggestions for the fix.
- Add _ = / _, _ = to all json.NewEncoder(w).Encode() and w.Write() calls in test handlers to satisfy gosec G104 (unhandled errors). - Replace require.NoError in httptest handler goroutines with error-check + WriteHeader(500) pattern (require calls FailNow which panics from non-test goroutines). - Run gofmt -s to fix formatting. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Both items from my previous review are addressed. The new commit:
- Adds
_ =/_, _ =to alljson.NewEncoder(w).Encode(...)andw.Write(...)calls, fixing the gosec G104 violations that were failing CI. - Replaces both
require.NoErrorcalls inside httptest handler goroutines with the error-check +WriteHeader(500)pattern.
Zero require calls remain inside httptest handlers. No new issues in the incremental diff. CI is running against fc884da.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Both items from my previous review are addressed. The new commit:
- Adds
_ =/_, _ =to alljson.NewEncoder(w).Encode(...)andw.Write(...)calls, fixing the gosec G104 violations that were failing CI. - Replaces both
require.NoErrorcalls inside httptest handler goroutines with the error-check +WriteHeader(500)pattern.
Zero require calls remain inside httptest handlers. No new issues in the incremental diff. CI is running against fc884da.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Two nit-level observations, neither blocking.
- TestListRoutineRuns_WithTop: add atomic.Int32 counter to assert only 1 HTTP request is made when Top caps results on the first page. - TestDisableRoutine_ServerError: add server error test for symmetry with TestEnableRoutine_ServerError. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Both nits from my last review are addressed in 6e47e23:
- TestListRoutineRuns_WithTop now uses an �tomic.Int32 counter and asserts exactly one HTTP call is made, confirming the client respects Top without over-fetching.
- TestDisableRoutine_ServerError closes the symmetry gap with EnableRoutine.
All earlier blocking items (gosec G104,
equire in handler goroutines) remain fixed. No new issues in this commit. CI is in progress with no failures.
My earlier CHANGES_REQUESTED should be cleared to unblock merge.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Both nits from my last review are addressed in 6e47e23:
- TestListRoutineRuns_WithTop now uses an atomic.Int32 counter and asserts exactly one HTTP call is made, confirming the client respects Top without over-fetching.
- TestDisableRoutine_ServerError closes the symmetry gap with EnableRoutine.
All earlier blocking items (gosec G104, require in handler goroutines) remain fixed. No new issues in this commit. CI is in progress with no failures.
My earlier CHANGES_REQUESTED should be cleared to unblock merge.
trangevi
dismissed
jongio’s stale review
Dismissing "changes requested" review as mentioned in last comment
Four recurring themes promoted from PR reviews merged since 2026-06-22: 1. go.instructions.md: Test goroutine safety — t.Fatal/require.NoError must not be called from httptest handler goroutines or other non-test goroutines; HTTP headers must be set before WriteHeader. (Source: #8790) 2. extensions.instructions.md: Duplicate helper detection — export shared helpers from the owning package instead of copying function bodies across extension packages. (Source: #8794, #8809) 3. extensions.instructions.md: User-provided path validation — paths from azure.yaml fields (instructions, entryPoint, etc.) must be validated to stay within the expected root directory. (Source: #8779, #8599) 4. extensions.instructions.md: Cross-extension test isolation — tests loading sibling-extension files must skip when absent; extension functional tests should have dedicated CODEOWNERS entries. (Source: #8809, #8754) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Why
The initial
azd ai routineextension PR (#8241) shipped without unit tests for the mission-critical data-plane client and with a minor code-hygiene nit (_ = MarkHiddenwith no explanation). Issue #8365 tracked these as follow-up items.What this PR does
Adds
client_test.go-- 28 httptest-based unit tests covering the fullroutines.Clientsurface:GetRoutine,ListRoutines(single and multi-page pagination),PutRoutine,DeleteRoutine,EnableRoutine,DisableRoutine,DispatchRoutineAsync,ListRoutineRuns(pagination with Top cap, filter parameter), URL construction helpers, and context cancellation.Adds
routine_helpers_test.go-- unit tests for theboolStrandsortedKeyshelper functions that were previously untested.Clarifies the
MarkHiddendiscard inroutine_update.gowith a comment explaining why ignoring the error is safe (the flags are registered on the samecmdimmediately above, so the call cannot fail).Items assessed but deferred
runtime.NewResponseErrorleaking raw body): This is the standard azcore pattern. Command-layer callers already wrap errors viaexterrors.ServiceFromAzure, which extracts structured fields before surfacing to users.armappservice/v2indirect dep): Benign cascade from the azd core module; adds no runtime weight.Fixes: #8365