Add team label tests to replace TODOs from PR #37208#37669
Conversation
Turns out, we have a lot to unwind here so this doesn't compile yet
Zed + Opus 4.5. Prompt below: Try to run `make test` and fix compile errors where the interface no longer matches the implementation until the compile completes. Don't change the associated interfaces, and use commit 5ad3c18 as a guide for the sort of changes that need to be made.
Courtesy Junie. Prompt: Using PlatformField.tsx as a pattern, give me a text field component (not an input, not editable) for "Team name". Include this in both ManualLabelForm and DynamicLabelForm, showing the component only if a new teamName prop (nullable string, required) is truthy. Pass this property all the way from LabelForm. There, add team_name to ILabel as an optional nullable string, and pass that to the DynamicLabelForm and ManualLabelForm components. TODO: A bunch of manual fixes, but figured I'd split those into a commit to show what's mine/what the robot did.
…scription, include teams reference
Backend isn't built here yet
… controls on label edit page and manage labels action menu
Used Zed + Opus 4.5 with the prompt: I'm getting test failures on frontend tests (`make test-js`) because I'm pretty sure real server requests are leaking through HTTP mocks. Fix the issues. It made some other changes that weren't relevant for fixing the problem, so I reverted those prior to committing.
Still TODO: * Database query changes * Filtering individual labels by team (including authorship) * Filtering associated hosts on edit
Thought I could use this for labels work but I can't
Zed + Opus 4.5. Prompt: Clean up testLabelsSummary work added in the most recent commit to iterate over a table of test cases rather than duplicating a bunch of test code. Name test cases based on the comments above each subtest.
Zed + Opus 4.5. Prompt: Infer expectedCount from expectedTeamLabels (expectedCount will always be 5 + len(expectedTeamLabels). Also remove default values from the testCases struct declarations, then inline the testCases variable rather than assigning to a temporary variable only used in the loop.
Still need to move auth checks around, which I'll have to do in part at the data store level so we don't load a label that isn't permitted into memory only to 403 after we grab it.
Co-authored-by: iansltx <472804+iansltx@users.noreply.github.com>
Co-authored-by: iansltx <472804+iansltx@users.noreply.github.com>
Co-authored-by: iansltx <472804+iansltx@users.noreply.github.com>
Co-authored-by: iansltx <472804+iansltx@users.noreply.github.com>
|
@coderabbitai full-review |
|
@iansltx: I'll conduct a full review of the PR changes. 🧠 Learnings used✅ Actions performedFull review triggered. |
WalkthroughThe PR adds comprehensive test coverage for team-scoped labels across the datastore, service integration, and authorization layers, validating label creation, retrieval, deletion, membership updates, and access control for both global and team-specific contexts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. 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: 0
🧹 Nitpick comments (7)
server/service/labels_test.go (1)
215-216: Unused variable:isTeam1Observer.The variable
isTeam1Observeris calculated but never used in the test assertions. Consider removing it since the read/write/delete permission checks don't reference it.🔎 Proposed cleanup
for _, t := range tt.user.Teams { if t.Team.ID == team1.ID { hasTeam1Access = true if t.Role == fleet.RoleMaintainer || t.Role == fleet.RoleAdmin { isTeam1Maintainer = true - } else if t.Role == fleet.RoleObserver { - isTeam1Observer = true } break } }server/service/integration_core_test.go (2)
5161-5258: Team label integration tests look good; double‑check host identifiers in manual label specThe new “Team Labels” and team label specs tests exercise the right behavior:
- Teams and team‑scoped labels are created via
ApplyLabelSpecs.TeamIDis asserted both when listing labels/specs and when fetching by ID/name.- Label deletion paths for team labels are covered.
One thing to verify: in the manual team label spec for
team2,Hostsis populated withfmt.Sprint(team2Hosts[0].ID)(a DB ID string). Elsewhere in this file, manual label membership APIs use UUID/hostname/node key/serial as host identifiers rather than DB IDs. IfApplyLabelSpecs’s host resolution does not explicitly support numeric DB IDs, this could:
- Fail with a 4xx because the host can’t be found, or
- Behave differently from the other manual-label entry points.
To keep host identification consistent and robust, consider switching that to a UUID or hostname, e.g.
team2Hosts[0].UUIDorteam2Hosts[0].Hostname, and update expectations if needed.Also applies to: 5485-5535
13929-13964: Team label add/remove flow is covered; consider extending cross‑team coverageThe added block in
TestAddingRemovingManualLabelsusefully verifies that:
- A team‑scoped manual label can be added to a host in that team.
- Attempting to add the same team label to a global host returns
422 Unprocessable Entity.- Removing the team label from the team host works and leaves no residual labels.
This matches the intended isolation semantics for team labels. If you want to harden the tests further, you could optionally add a case that tries to apply a team1 label to a host in a different team and asserts the same rejection path, to ensure cross‑team enforcement is symmetrical.
server/datastore/mysql/labels_test.go (4)
293-424: Inconsistent context usage in the test.The test defines
ctx := context.Background()at line 293 but several subsequent calls still usecontext.Background()directly (e.g., lines 301, 314, 327, etc.). While this doesn't affect test correctness, it's inconsistent with the pattern established at the start of the test additions.🔎 Suggested fix for consistency
- h1, err := db.NewHost(context.Background(), &fleet.Host{ + h1, err := db.NewHost(ctx, &fleet.Host{Apply similar changes to h2, h3 creation and other
context.Background()calls within this function.
1040-1041: Stale TODO comment should be removed.The TODO comment at line 1041 states "TODO test team label filtering" but team label filtering tests have now been implemented in this function (lines 1102-1129). Consider removing this stale comment.
🔎 Proposed fix
func testDeleteLabel(t *testing.T, db *Datastore) { - // TODO test team label filtering - ctx := context.Background()
2037-2037: Stale TODO comment should be removed.The TODO at line 2037 states "TODO validate team label host validation behavior" but this behavior has now been tested in the additions at lines 2203-2262. Consider removing this stale comment.
🔎 Proposed fix
func testUpdateLabelMembershipByHostIDs(t *testing.T, ds *Datastore) { - // TODO validate team label host validation behavior - ctx := context.Background()
2820-2821: Empty test function with TODO.The
testSetAsideLabelsfunction is empty with just a TODO comment. If this test case is registered in the test table (line 106), it will pass without testing anything.Would you like me to help implement this test, or should this be tracked as a separate issue? If intentionally deferred, consider adding a more descriptive TODO comment explaining what needs to be tested.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/datastore/mysql/labels_test.goserver/service/integration_core_test.goserver/service/labels_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/service/labels_test.goserver/datastore/mysql/labels_test.goserver/service/integration_core_test.go
🧠 Learnings (5)
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
Learning: In fleetdm/fleet repository tests (server/datastore/mysql/labels_test.go and similar), using testing.T.Context() is valid because the project targets a recent Go version where testing.T.Context() exists. Do not suggest replacing t.Context() with context.Background() in this codebase.
Applied to files:
server/service/labels_test.goserver/datastore/mysql/labels_test.goserver/service/integration_core_test.go
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
Learning: Fleet repo targets Go 1.24.5 (root go.mod), which supports testing.T.Context(). Do not flag usage of t.Context() or suggest replacing it with context.Background() in tests (e.g., server/datastore/mysql/labels_test.go Line 2031 and similar).
Applied to files:
server/datastore/mysql/labels_test.go
📚 Learning: 2025-10-03T18:16:11.482Z
Learnt from: MagnusHJensen
Repo: fleetdm/fleet PR: 33805
File: server/service/integration_mdm_test.go:1248-1251
Timestamp: 2025-10-03T18:16:11.482Z
Learning: In server/service/integration_mdm_test.go, the helper createAppleMobileHostThenEnrollMDM(platform string) is exclusively for iOS/iPadOS hosts (mobile). Do not flag macOS model/behavior issues based on changes within this helper; macOS provisioning uses different helpers such as createHostThenEnrollMDM.
Applied to files:
server/datastore/mysql/labels_test.go
📚 Learning: 2025-08-08T08:32:31.529Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31695
File: server/datastore/mysql/apple_mdm_test.go:132-132
Timestamp: 2025-08-08T08:32:31.529Z
Learning: Datastore.NewMDMWindowsConfigProfile signature is: NewMDMWindowsConfigProfile(ctx context.Context, cp fleet.MDMWindowsConfigProfile, usesFleetVars []string) (*fleet.MDMWindowsConfigProfile, error). Passing nil for usesFleetVars in tests denotes “no Fleet variables referenced” and is used consistently across the repo.
Applied to files:
server/datastore/mysql/labels_test.go
📚 Learning: 2025-08-13T18:20:42.136Z
Learnt from: titanous
Repo: fleetdm/fleet PR: 31075
File: tools/redis-tests/elasticache/iam_auth.go:4-10
Timestamp: 2025-08-13T18:20:42.136Z
Learning: For test harnesses and CLI tools in the Fleet codebase, resource cleanup on error paths (like closing connections before log.Fatalf) may not be necessary since the OS handles cleanup when the process exits. These tools prioritize simplicity over defensive programming patterns used in production code.
Applied to files:
server/service/integration_core_test.go
🧬 Code graph analysis (3)
server/service/labels_test.go (3)
server/fleet/teams.go (2)
Team(69-95)TeamFilter(579-588)server/mock/datastore_mock.go (2)
NewLabelFunc(156-156)LabelFunc(164-164)server/fleet/labels.go (1)
LabelWithTeamName(157-160)
server/datastore/mysql/labels_test.go (2)
server/fleet/teams.go (2)
Team(69-95)TeamFilter(579-588)server/fleet/labels.go (4)
LabelSpec(223-234)Label(142-155)LabelType(61-61)LabelMembershipType(95-95)
server/service/integration_core_test.go (2)
server/fleet/hosts.go (1)
NewAddHostsToTeamParams(1558-1564)server/fleet/labels.go (3)
LabelSpec(223-234)LabelMembershipTypeManual(102-102)LabelMembershipTypeDynamic(100-100)
🔇 Additional comments (7)
server/service/labels_test.go (2)
121-152: LGTM! Team label setup and mocks are correctly configured.The team label and mocks are properly set up to support the new permission tests. The mock functions correctly preserve existing test cases while adding the new team-scoped label case.
222-238: Excellent permission test coverage!The team label permission tests correctly validate the authorization matrix:
- Global users can read all team labels
- Global admins/maintainers can modify/delete all team labels
- Team members can read their team's labels
- Team admins/maintainers can modify/delete their team's labels
- Team observers have read-only access to their team's labels
The permission logic is sound and aligns with the PR objectives for validating team isolation boundaries.
server/datastore/mysql/labels_test.go (5)
766-805: LGTM!The team label specs test correctly validates:
- Team labels are created and retrieved with proper team filtering
- Global specs do not include team labels
- Team1 and Team2 specs are isolated from each other
816-845: LGTM!The test properly validates:
- Team labels can be looked up by name with the correct team filter
- Global filter correctly excludes team labels
- Label IDs are correctly returned
872-903: LGTM!The test provides good coverage for
LabelsByNamewith team labels, including validation that label descriptions are preserved and that global filters properly exclude team labels.
1102-1129: LGTM!The test correctly validates team label deletion behavior:
- Deletion with global filter fails with NotFoundError (team label not visible in global scope)
- Deletion with correct team filter succeeds
- Subsequent lookup confirms the label is deleted
This properly tests the team isolation boundary for delete operations.
2203-2262: LGTM - Critical team isolation test.This test validates an important security boundary: hosts from one team cannot be added to another team's label. The test correctly:
- Creates hosts in separate teams
- Creates a team-scoped label
- Validates that adding a host from the correct team succeeds
- Validates that adding a host from a different team is filtered out (returns empty hostIDs)
This is essential for ensuring team isolation is enforced at the datastore level.
|
@copilot take CodeRabbit feedback into account |
…ext usage Co-authored-by: iansltx <472804+iansltx@users.noreply.github.com>
I've addressed the CodeRabbit feedback in commit a2a1100:
All tests compile successfully. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #37669 +/- ##
==========================================
- Coverage 65.88% 57.36% -8.53%
==========================================
Files 2360 2361 +1
Lines 187285 187383 +98
Branches 8006 8017 +11
==========================================
- Hits 123402 107496 -15906
- Misses 52605 69479 +16874
+ Partials 11278 10408 -870
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Going to regen this since a lot has changed. |
PR #37208 introduced team labels API but left TODOs for missing test coverage. This PR implements comprehensive tests for team label functionality across datastore, service, and integration layers.
Changes
Datastore tests (
server/datastore/mysql/labels_test.go):Service layer tests (
server/service/labels_test.go):Integration tests (
server/service/integration_core_test.go):All tests follow existing patterns and validate team isolation boundaries.
Code quality improvements (based on automated code review feedback):
require.NoError(t, err)instead ofrequire.Nil(t, err)ctxvariable instead ofcontext.Background()Checklist for submitter
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.