test: add unit tests for key server/models validation types#230
test: add unit tests for key server/models validation types#230mason5052 wants to merge 2 commits intovxcontrol:mainfrom
Conversation
Add comprehensive test coverage for the server/models package validation logic including enum types, struct validators, and custom validators. Test files added: - users_test.go: UserStatus, UserType, Login, Password, User, UserPassword, AuthCallback, UserRole, UserPreferences validation - providers_test.go: ProviderType, Provider, CreateProvider, PatchProvider, ProviderInfo validation - api_tokens_test.go: TokenStatus, APIToken, CreateAPITokenRequest, UpdateAPITokenRequest, APITokenClaims validation - flows_test.go: FlowStatus, TaskStatus, SubtaskStatus, ContainerStatus, ContainerType, Role, Privilege, RolePrivileges, PatchFlow validation - init_test.go: Custom validators (stpass, vmail, oauth_min_scope, solid, semver, semverex) and scanFromJSON helper
There was a problem hiding this comment.
Pull request overview
Adds unit test coverage for backend/pkg/server/models validation logic to ensure core model validators (users, providers, flows, API tokens, and custom validators) are exercised and regressions are caught early.
Changes:
- Introduces table-driven unit tests for enum/string validators and
Valid()methods across key models. - Adds boundary/negative-case tests for security-sensitive validation rules (password strength, OAuth scopes, JWT-related claims).
- Adds tests for helper/validator initialization and JSON scan helpers.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/pkg/server/models/users_test.go | Adds tests for user/login/password models, role linking, and preferences JSON scan/value behavior. |
| backend/pkg/server/models/providers_test.go | Adds tests for provider type validation and provider create/patch/info payload validation. |
| backend/pkg/server/models/api_tokens_test.go | Adds tests for API token status/struct validation, TTL boundaries, and claims validation. |
| backend/pkg/server/models/flows_test.go | Adds tests for flow/task/subtask/container status/type validation and patch action validation. |
| backend/pkg/server/models/init_test.go | Adds tests for custom validators (stpass, vmail, oauth_min_scope, solid, semver, semverex) and scanFromJSON. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
| err := tt.status.Valid() |
There was a problem hiding this comment.
Subtest uses t.Parallel() inside a for-range over tt; in Go the range variable is reused, so parallel subtests can race and all observe the last tt value. Shadow tt := tt inside the loop (or pass tt as an argument) before calling t.Run.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
| err := tt.userType.Valid() |
There was a problem hiding this comment.
Subtest uses t.Parallel() while closing over the for-range variable tt. Because tt is reused across iterations, parallel subtests can read the wrong case. Add tt := tt inside the loop before t.Run.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
| err := tt.login.Valid() |
There was a problem hiding this comment.
t.Run subtest calls t.Parallel() and captures the loop variable tt from a for-range; this can cause flaky tests due to loop-variable reuse. Shadow tt := tt (or pass as param) inside the loop before launching the subtest.
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { |
There was a problem hiding this comment.
Parallel subtests capture the for-range variable tt (reused each iteration). With t.Parallel(), this can make subtests execute with the wrong inputs. Fix by shadowing tt := tt inside the loop before t.Run.
| for _, tt := range tests { | |
| for _, tt := range tests { | |
| tt := tt |
| assert.Error(t, up.Valid()) | ||
| assert.Contains(t, up.Valid().Error(), "user_id is required") |
There was a problem hiding this comment.
This assertion calls up.Valid() twice; the second call is unnecessary and can hide issues if Valid() becomes stateful. Store the error once (e.g., err := up.Valid()) and assert on err/err.Error().
| assert.Error(t, up.Valid()) | |
| assert.Contains(t, up.Valid().Error(), "user_id is required") | |
| err := up.Valid() | |
| assert.Error(t, err) | |
| assert.Contains(t, err.Error(), "user_id is required") |
| {"invalid unknown", TaskStatus("cancelled"), true}, | ||
| } | ||
|
|
||
| for _, tt := range tests { |
There was a problem hiding this comment.
Parallel subtests capture the loop variable tt from a for-range. Since tt is reused, this can cause flaky/incorrect subtest inputs. Add tt := tt inside the loop before t.Run.
| for _, tt := range tests { | |
| for _, tt := range tests { | |
| tt := tt |
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
| err := tt.status.Valid() |
There was a problem hiding this comment.
t.Run subtests use t.Parallel() and capture the for-range variable tt (reused). Shadow tt := tt inside the loop before t.Run to ensure each subtest gets correct data.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
| err := tt.status.Valid() |
There was a problem hiding this comment.
Parallel subtests capture for-range variable tt (reused each iteration), which can lead to incorrect ContainerStatus test cases executing. Shadow tt := tt inside the loop before t.Run.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
| err := tt.ct.Valid() |
There was a problem hiding this comment.
This for-range launches parallel subtests that close over tt; since Go reuses the range variable, subtests can run with the wrong ContainerType values. Add tt := tt inside the loop before calling t.Run.
| {"invalid deleted", TokenStatus("deleted"), true}, | ||
| } | ||
|
|
||
| for _, tt := range tests { |
There was a problem hiding this comment.
Parallel subtests capture the for-range variable tt (reused). With t.Parallel(), this can cause cases to execute with the wrong status. Shadow tt := tt inside the loop before t.Run.
| for _, tt := range tests { | |
| for _, tt := range tests { | |
| tt := tt |
Add missing test coverage identified by cross-review: New files: - assistants_test.go: AssistantStatus enum, Assistant struct, CreateAssistant, PatchAssistant (stop/input actions), AssistantFlow nested validation - prompts_test.go: PromptType enum (8 valid constants from templates pkg), Prompt struct, PatchPrompt validation Extended existing files: - users_test.go: add AuthCallback positive-path test, UserRolePrivileges validation, fix brittle double-call assertion in UserPreferences test - api_tokens_test.go: add APITokenWithSecret validation (valid, invalid embedded token, invalid JWT) - flows_test.go: add Flow.Valid, FlowTasksSubtasks.Valid, FlowContainers.Valid, Task.Valid, TaskSubtasks.Valid, Subtask.Valid, Container.Valid with valid/invalid/missing-field cases
mason5052
changed the title
|
hey @mason5052 there are correct notice from Copilot side, you need to change these syntax constructions to make immutable |
What
Add unit tests for validation logic across key
server/modelstypes: users, providers, API tokens, flows, tasks, containers, assistants, prompts, roles, and custom validators.This does not cover every exported validator in the package (log models, analytics, msgchains, screenshots, and vecstore models are not included).
Why
These model types contain security-sensitive validation functions (password strength, OAuth scope, JWT claims, API token TTL bounds) and core domain validation (enum status types, struct field requirements, nested model validation). None had test coverage.
Test Files
users_test.goproviders_test.goapi_tokens_test.goflows_test.goinit_test.goassistants_test.goprompts_test.goTest Approach
t.Parallel()for fast executionvalidateandscanFromJSONVerification