Clean up Gitops tests and add deprecation tests by sgress454 · Pull Request #43039 · fleetdm/fleet

sgress454 · 2026-04-06T02:50:58Z

Related issue: For #40015

Moves repeated empty mocks into a new setupEmptyGitOpsMocks method
Adds new "deprecation" tests:
- In TestGitOpsFullGlobal, TestGitOpsFullTeam and TestGitOpsFullGlobalAndTeam tests "kitchen sink" with both new and deprecated keys
- Added keys and checks to verify setup_experience, apple_business_manager and volume_purchasing_program configs
Consolidated map of deprecated -> new GitOps keys in one place

sgress454 · 2026-04-06T02:51:21Z

cmd/fleetctl/fleetctl/testdata/gitops/lib/signed.pkg

Added to test bootstrap_package and macos_bootstrap_package

sgress454 · 2026-04-06T02:52:08Z

cmd/fleetctl/fleetctl/testdata/gitops/global_config_no_paths.yml

updated all keys in here and copied the original to global_config_no_paths_deprecated_keys.yml

sgress454 · 2026-04-06T02:52:44Z

cmd/fleetctl/fleetctl/testdata/gitops/team_config_no_paths.yml

updated keys and copied original to team_config_no_paths_deprecated_keys.yml

sgress454 · 2026-04-06T02:53:17Z

cmd/fleetctl/fleetctl/testing_utils/testing_utils.go

+				case strings.HasSuffix(r.URL.Path, ".pkg"):
+					pkgDir := getPathRelative("../testdata/gitops/lib/")
+					http.ServeFile(w, r, filepath.Join(pkgDir, filepath.Base(r.URL.Path)))


to mock serve the setup experience script to test macos_setup.script / setup_experience.script

sgress454 · 2026-04-06T02:54:15Z

cmd/fleetctl/fleetctl/testing_utils/testing_utils.go

+	ds.InsertMDMAppleBootstrapPackageFunc = func(ctx context.Context, bp *fleet.MDMAppleBootstrapPackage, pkgStore fleet.MDMBootstrapPackageStore) error {
+		return nil
+	}


new mock for us to spy on to test that macos_setup.boostrap_package / setup_experience.macos_bootstrap_package work

cmd/fleetctl/fleetctl/testing_utils/testing_utils.go

sgress454 · 2026-04-06T03:11:55Z

cmd/fleetctl/fleetctl/generate_gitops.go

We were maintaining (lazily) the entire set of renamed keys in here to guard against missing any, but we now have a better set of deprecated keys maintained in pkg/spec/gitops_deprecations.go, so i'm just having generate_gitops derive its renaming rules from that list.

I spot-checked this and found one missing (android config profiles), then I told Claude to run through all calls to validateRawKeys or validateYAMLKeys which should provide a pretty comprehensive list of GitOps structs, then find keys in those structs with renames. The only missing ones were things we don't actually use in GitOps (like labels.team_id).

If we did miss something in this list, the result would be missing a deprecation warning and not having old/new key conflict detection. It's not a risk of an old or new key not working. So I think it's worth the cleanup to keep these from drifting.

sgress454 · 2026-04-06T03:12:52Z

cmd/fleetctl/fleetctl/gitops_test.go

+			assert.False(t, savedTeam.Config.MDM.MacOSSetup.EnableReleaseDeviceManually.Value)
+			assert.False(t, savedTeam.Config.MDM.MacOSSetup.ManualAgentInstall.Value)
+			assert.True(t, ds.SetSetupExperienceScriptFuncInvoked)
+			assert.True(t, ds.InsertMDMAppleBootstrapPackageFuncInvoked)


This validates that the bootstrap_package / macos_bootstrap_package key works

sgress454 · 2026-04-06T03:13:46Z

cmd/fleetctl/fleetctl/gitops_test.go

+			assert.False(t, savedTeam.Config.MDM.MacOSSetup.EnableReleaseDeviceManually.Value)
+			assert.False(t, savedTeam.Config.MDM.MacOSSetup.ManualAgentInstall.Value)
+			assert.True(t, ds.SetSetupExperienceScriptFuncInvoked)


This validates that the other setup experience keys are checked

sgress454 · 2026-04-06T03:16:05Z

cmd/fleetctl/fleetctl/gitops_test.go

-		assert.Equal(t, []uint{policy.ID}, IDs)
-		return []uint{policy.ID}, nil


moved this assertion to the main test block

sgress454 · 2026-04-06T03:16:20Z

cmd/fleetctl/fleetctl/gitops_test.go

 	ds.ListQueriesFunc = func(ctx context.Context, opts fleet.ListQueryOptions) ([]*fleet.Query, int, int, *fleet.PaginationMetadata, error) {
 		return []*fleet.Query{&query}, 1, 0, nil, nil
 	}
 	ds.DeleteQueriesFunc = func(ctx context.Context, ids []uint) (uint, error) {
 		queryDeleted = true
-		assert.Equal(t, []uint{query.ID}, ids)


moved this assertion to the main test block

codecov · 2026-04-06T03:25:17Z

Codecov Report

❌ Patch coverage is 67.94872% with 50 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.86%. Comparing base (1e24ead) to head (90c776d).
⚠️ Report is 98 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/fleetctl/fleetctl/testing_utils.go	63.76%	24 Missing and 26 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #43039      +/-   ##
==========================================
- Coverage   66.86%   66.86%   -0.01%     
==========================================
  Files        2578     2578              
  Lines      206869   207053     +184     
  Branches     9168     9168              
==========================================
+ Hits       138328   138443     +115     
- Misses      55978    56014      +36     
- Partials    12563    12596      +33

Flag	Coverage Δ
backend	`68.63% <67.94%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

sgress454 · 2026-04-06T13:04:37Z

cmd/fleetctl/fleetctl/gitops_test.go

The majority of changes here are just removing mocks in favor of calling setupEmptyGitOpsMocks(ds). In a couple of places, assertions in the mocks have been moved to the tests.

The other changes are the wrapping of some test code in loops to test with both new and deprecated keys, which also involves hoisting some testing vars up.

Lastly, we add a couple of new assertions to test things like setup experience keys. These have been commented separately.

sgress454 · 2026-04-06T13:06:57Z

cmd/fleetctl/fleetctl/testing_utils.go

+// or return specific values.
+//
+//nolint:unused // used in tests
+func setupEmptyGitOpsMocks(ds *mock.Store) {


There may be some overlap here with mocks set up in RunServerWithMockedDS(). Just moving the rest of these into that method would be an option, but it's used in other places so this felt like the lightest touch.

claude

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

_{Tip: disable this comment in your organization's Code Review settings.}

Copilot

Pull request overview

This PR cleans up fleetctl gitops test scaffolding and expands coverage for recently renamed/deprecated GitOps YAML keys by centralizing deprecated→new key mappings and adding fixtures/tests that exercise both canonical and deprecated forms.

Changes:

Centralized deprecated GitOps key mappings in pkg/spec and derived generate-gitops alias rules from that single source of truth.
Refactored fleetctl gitops tests to use a shared setupEmptyGitOpsMocks helper and added new fixtures for deprecated-key “kitchen sink” runs.
Updated GitOps YAML fixtures to use canonical keys (settings, reports, setup_experience, apple_settings, server_settings report fields) and added a .pkg test fixture served by the installer test server.

Reviewed changes

Copilot reviewed 10 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
pkg/spec/testdata/org-settings.yml	Updates org settings fixture to canonical report-related server_settings keys.
pkg/spec/gitops_test.go	Updates assertions for canonical org settings key names/types.
pkg/spec/gitops_deprecations.go	Adds/centralizes deprecated→canonical GitOps key mappings and warning behavior.
cmd/fleetctl/fleetctl/generate_gitops.go	Builds output alias rules from the centralized mappings.
cmd/fleetctl/fleetctl/testing_utils/testing_utils.go	Extends test installer server to serve `.pkg` and adds missing datastore mock.
cmd/fleetctl/fleetctl/testing_utils.go	Introduces `setupEmptyGitOpsMocks` to reduce duplicated gitops datastore mocking.
cmd/fleetctl/fleetctl/gitops_test.go	Refactors tests to use shared mocks and adds runs for deprecated vs canonical keys.
cmd/fleetctl/fleetctl/testdata/gitops/*.yml	Updates canonical fixtures and adds deprecated-key variants for coverage.
cmd/fleetctl/fleetctl/testdata/gitops/lib/signed.pkg	Adds a `.pkg` fixture used by setup experience/bootstrap package tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-06T13:15:52Z

cmd/fleetctl/fleetctl/gitops_test.go

+			t.Setenv("FLEET_SERVER_URL", fleetServerURL)
+			t.Setenv("ORG_NAME", orgName)
+			t.Setenv("SOFTWARE_INSTALLER_URL", fleetServerURL)
+			t.Setenv("FLEET_ENABLE_LOG_TOPICS", "deprecated-field-names")
+
+			// Dry run w/ top-level labels key
+			logs := RunAppForTest(t, []string{"gitops", "-f", path, "--dry-run"})
+			fmt.Printf("%s", logs)


The new subtests enable the deprecated-field-names log topic but never assert on the output. To make these true deprecation tests, assert that the warning text is present when useDeprecatedKeys==true (and absent when false) so the test fails if warnings stop being emitted.

cmd/fleetctl/fleetctl/gitops_test.go

Copilot · 2026-04-06T13:15:53Z

cmd/fleetctl/fleetctl/testing_utils.go

+//
+//nolint:unused // used in tests


setupEmptyGitOpsMocks is referenced from tests in this package, so the //nolint:unused suppression is redundant and can mask future dead code. Consider removing the nolint directive so the linter will catch it if the helper stops being used.

Suggested change

//

//nolint:unused // used in tests

sgress454 · 2026-04-06T13:20:44Z

pkg/spec/gitops_test.go

Since we added new entries to ApplyDeprecatedKeyMappings, conflicts for those keys are caught earlier (during file parsing rather than during the control: section processing), but they use the full paths rather than just the key names, hence the test updates

iansltx

Got to L1722 in the new version of gitops_test.go. Checkpointing so I don't lose my place. One piece of feedback so far; some of these refactors made the mocks that weren't swalloped up in the default mocks more consistent, which is nice.

iansltx · 2026-04-06T22:49:51Z

cmd/fleetctl/fleetctl/generate_gitops.go

+		// Take the last segment of the old and new paths as the leaf key names.
+		// Remove any array indicators (e.g. "[]") in case an array key is renamed.
+		oldLeaf := m.OldPath
+		if i := strings.LastIndex(oldLeaf, "."); i >= 0 {


Does this work properly with query?

You mean paths without .? It should, the i >= 0 will fail (not found is -1, just like JS), so oldLeaf will remain unchanged.

Yeah, thinking in terms of false-positive replacing old -> new when we shouldn't, given that this strips initial path components when they exist.

Ah yeah for query specifically we just don't alias that, as it's not applicable anywhere in gitops (i.e. there's no query field in gitops config that doesn't refer to an actual SQL query).

iansltx

Given that my concern about the query case is misplaced, this looks fine. Had to trust that tests were equivalent when thrown in the loop because diffs were inaccurate, but given that test length was comparable and we didn't have code changes for that part of the code that seems like a valid assumption.

**Related issue:** For #40015 * Moves repeated empty mocks into a new `setupEmptyGitOpsMocks` method * Adds new "deprecation" tests: * In TestGitOpsFullGlobal, TestGitOpsFullTeam and TestGitOpsFullGlobalAndTeam tests "kitchen sink" with both new and deprecated keys * Added keys and checks to verify `setup_experience`, `apple_business_manager` and `volume_purchasing_program` configs * Consolidated map of deprecated -> new GitOps keys in one place

…43255) Cherry-pick of #43039 into the RC branch.

**Related issue:** For #40015 * Moves repeated empty mocks into a new `setupEmptyGitOpsMocks` method * Adds new "deprecation" tests: * In TestGitOpsFullGlobal, TestGitOpsFullTeam and TestGitOpsFullGlobalAndTeam tests "kitchen sink" with both new and deprecated keys * Added keys and checks to verify `setup_experience`, `apple_business_manager` and `volume_purchasing_program` configs * Consolidated map of deprecated -> new GitOps keys in one place

sgress454 temporarily deployed to Docker Hub April 6, 2026 02:51 — with GitHub Actions Inactive

sgress454 commented Apr 6, 2026

View reviewed changes

github-advanced-security bot found potential problems Apr 6, 2026

View reviewed changes

cmd/fleetctl/fleetctl/testing_utils/testing_utils.go Dismissed Show dismissed Hide dismissed

sgress454 temporarily deployed to Docker Hub April 6, 2026 03:03 — with GitHub Actions Inactive

sgress454 commented Apr 6, 2026

View reviewed changes

sgress454 temporarily deployed to Docker Hub April 6, 2026 03:41 — with GitHub Actions Inactive

sgress454 temporarily deployed to Docker Hub April 6, 2026 03:50 — with GitHub Actions Inactive

sgress454 temporarily deployed to Docker Hub April 6, 2026 03:56 — with GitHub Actions Inactive

sgress454 temporarily deployed to Docker Hub April 6, 2026 04:11 — with GitHub Actions Inactive

sgress454 temporarily deployed to Docker Hub April 6, 2026 04:45 — with GitHub Actions Inactive

sgress454 commented Apr 6, 2026

View reviewed changes

sgress454 marked this pull request as ready for review April 6, 2026 13:07

sgress454 requested a review from a team as a code owner April 6, 2026 13:07

Copilot AI review requested due to automatic review settings April 6, 2026 13:07

claude bot reviewed Apr 6, 2026

View reviewed changes

Copilot started reviewing on behalf of sgress454 April 6, 2026 13:08 View session

sgress454 assigned iansltx Apr 6, 2026

Copilot AI reviewed Apr 6, 2026

View reviewed changes

sgress454 commented Apr 6, 2026

View reviewed changes

sgress454 mentioned this pull request Apr 6, 2026

Activate deprecation warnings #41449

Merged

6 tasks

sgress454 added 17 commits April 6, 2026 12:51

add more deprecated keys to map

ecff0be

derive rules from gitops deprecations

f8f7413

clean up

1240b58

add more deprecation tests

34524db

add tests for other keys

4701b79

script test

9b7a30f

bootstrap test

232e244

restore checks

b401bea

add android setting

c9856cd

test fixes

ad7875c

add back api rename rules

2a2009c

clean up

4d4a1dd

lint

cc0672e

test fix

1c51257

lint

db0aec5

lint

95633b2

address review feedback

bc950ad

sgress454 force-pushed the sgress454/factor-out-gitops-mocks branch from 1683fe7 to bc950ad Compare April 6, 2026 17:51

sgress454 temporarily deployed to Docker Hub April 6, 2026 17:51 — with GitHub Actions Inactive

update test

90c776d

sgress454 had a problem deploying to Docker Hub April 7, 2026 03:01 — with GitHub Actions Failure

iansltx reviewed Apr 8, 2026

View reviewed changes

iansltx approved these changes Apr 8, 2026

View reviewed changes

sgress454 temporarily deployed to Docker Hub April 8, 2026 14:58 — with GitHub Actions Inactive

sgress454 merged commit 3ae98ee into main Apr 8, 2026
54 of 55 checks passed

sgress454 deleted the sgress454/factor-out-gitops-mocks branch April 8, 2026 16:57

sgress454 mentioned this pull request Apr 8, 2026

Cherry-pick #43039: Clean up Gitops tests and add deprecation tests #43255

Merged

sgress454 added a commit that referenced this pull request Apr 8, 2026

Cherry-pick #43039: Clean up Gitops tests and add deprecation tests (#…

ce7ed3d

…43255) Cherry-pick of #43039 into the RC branch.

		assert.Equal(t, []uint{policy.ID}, IDs)
		return []uint{policy.ID}, nil

Conversation

sgress454 commented Apr 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgress454 Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claude bot left a comment

Choose a reason for hiding this comment

Claude Code Review

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iansltx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iansltx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sgress454 Apr 6, 2026 •

edited

Loading

codecov bot commented Apr 6, 2026 •

edited

Loading