Move mysql/testing_utils.go to a separate mysql/mysqltest package#45406
Merged
lucasmrod merged 5 commits intoMay 14, 2026
Merged
Conversation
…astore/mysql/mysqltest package
lucasmrod
changed the title
mysql/testing_utils.go to a separate mysql/mysqltest package
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #45406 +/- ##
==========================================
+ Coverage 66.71% 66.73% +0.02%
==========================================
Files 2734 2733 -1
Lines 218839 218444 -395
Branches 10911 10645 -266
==========================================
- Hits 145988 145786 -202
+ Misses 59626 59458 -168
+ Partials 13225 13200 -25
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:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors MySQL-related test helpers out of the production server/datastore/mysql package into a new server/datastore/mysql/mysqltest package, with the goal of keeping Go’s testing package out of Fleet’s production binaries (per #45220).
Changes:
- Introduces
server/datastore/mysql/mysqltestand updates many tests/benchmarks/integration tests to use it instead ofserver/datastore/mysql. - Adds narrow exported accessors in
server/datastore/mysqlto supportmysqltestwithout reintroducingtestingimports. - Moves replica inspection helpers (
MasterStatus/ReplicaStatus) into non-_test.gocode somysqltestcan use them.
Reviewed changes
Copilot reviewed 75 out of 75 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| server/worker/worker_test.go | Switches MySQL test helper usage to mysqltest. |
| server/worker/software_worker_test.go | Switches MySQL test helper usage to mysqltest. |
| server/worker/macos_setup_assistant_test.go | Switches MySQL test helper usage to mysqltest. |
| server/worker/db_migrations_test.go | Switches MySQL test helper usage to mysqltest. |
| server/worker/apple_mdm_test.go | Uses mysqltest for ad-hoc SQL and DS setup. |
| server/vulnerabilities/vulntest/vulnerability_scanning_test.go | Switches MySQL test helper usage to mysqltest. |
| server/vulnerabilities/vulntest/gen_fixture_test.go | Switches MySQL test helper usage to mysqltest. |
| server/vulnerabilities/oval/analyzer_test.go | Switches MySQL test helper usage to mysqltest (bench + tests). |
| server/vulnerabilities/macoffice/integration_analyzer_test.go | Switches MySQL test helper usage to mysqltest. |
| server/service/users_test.go | Uses mysqltest for DS setup/truncation while keeping mysql types. |
| server/service/testing_client_test.go | Uses mysqltest DS setup + ExecAdhocSQL for suite setup/teardown. |
| server/service/sessions_test.go | Uses mysqltest DS setup/truncation. |
| server/service/service_campaign_test.go | Uses mysqltest for aggregated stats + DS setup/truncation/replica. |
| server/service/schedule/schedule_test.go | Uses mysqltest DS setup. |
| server/service/queries_test.go | Uses mysqltest DS setup. |
| server/service/packs_test.go | Uses mysqltest DS setup/truncation. |
| server/service/orbit_test.go | Uses mysqltest DS setup/options + ad-hoc SQL. |
| server/service/microsoft_mdm_integration_test.go | Uses mysqltest DS setup + Windows profile fixture helper. |
| server/service/mdm_test.go | Uses mysqltest.GenerateTestCertBytes for cert fixtures. |
| server/service/labels_test.go | Uses mysqltest DS setup/options + truncation. |
| server/service/integrationtest/suite.go | Uses mysqltest.CreateMySQLDSWithOptions for integration setup. |
| server/service/integrationtest/android/suite.go | Uses mysqltest.TestDBConnections for integration server wiring. |
| server/service/integrationtest/android/android_test.go | Uses mysqltest.TruncateTables for per-test cleanup. |
| server/service/integration_vpp_install_test.go | Uses mysqltest.ExecAdhocSQL for DB assertions/mutations. |
| server/service/integration_sso_test.go | Uses mysqltest.ExecAdhocSQL for DB updates. |
| server/service/integration_software_titles_test.go | Uses mysqltest.ExecAdhocSQL for DB reads. |
| server/service/integration_mdm_setup_experience_test.go | Uses mysqltest.ExecAdhocSQL + DumpTable for debugging. |
| server/service/integration_mdm_release_worker_test.go | Uses mysqltest.TruncateTables + ExecAdhocSQL. |
| server/service/integration_mdm_profiles_test.go | Uses mysqltest for ad-hoc SQL and Windows profile fixtures. |
| server/service/integration_mdm_lifecycle_test.go | Uses mysqltest.ExecAdhocSQL for DB mutations during lifecycle tests. |
| server/service/integration_mdm_dep_test.go | Uses mysqltest.ExecAdhocSQL for job manipulation. |
| server/service/integration_mdm_ddm_test.go | Uses mysqltest.ExecAdhocSQL for DDM fixture creation/assertions. |
| server/service/integration_mdm_commands_test.go | Uses mysqltest.ExecAdhocSQL for DB timestamp backdating. |
| server/service/integration_live_queries_test.go | Uses mysqltest.ExecAdhocSQLWithError and GetAggregatedStats. |
| server/service/integration_install_test.go | Uses mysqltest.ExecAdhocSQL for DB assertions. |
| server/service/integration_enterprise_vulns_test.go | Uses mysqltest.ExecAdhocSQL for vuln DB checks. |
| server/service/integration_desktop_test.go | Uses mysqltest.ExecAdhocSQL for device-auth DB setup. |
| server/service/integration_core_test.go | Uses mysqltest.ExecAdhocSQL + NewTestActivityService helpers. |
| server/service/integration_certificate_authorities_test.go | Uses mysqltest.ExecAdhocSQL for cleanup. |
| server/service/integration_android_software_test.go | Uses mysqltest.ExecAdhocSQL for activity/self_service assertions. |
| server/service/integration_android_certificate_templates_test.go | Uses mysqltest.ExecAdhocSQL for host_mdm + template assertions. |
| server/service/hosts_test.go | Uses mysqltest DS setup + activity helpers (NewTestActivityService, ListActivitiesAPI). |
| server/service/async/async_test.go | Uses mysqltest DS setup + truncation across subtests. |
| server/service/async/async_scheduled_query_stats_test.go | Uses mysqltest.ExecAdhocSQL for DB selects in tests. |
| server/service/async/async_policy_test.go | Uses mysqltest.ExecAdhocSQL for DB selects/inserts in tests. |
| server/service/async/async_label_test.go | Uses mysqltest.ExecAdhocSQL for DB selects in tests. |
| server/service/async/async_host_seen_test.go | Uses mysqltest.ExecAdhocSQL for DB selects in tests. |
| server/service/async/async_bench_test.go | Uses mysqltest DS setup + truncation + ad-hoc SQL in benchmarks. |
| server/service/apple_mdm_test.go | Uses mysqltest.GenerateTestCertBytes for APNS cert fixtures. |
| server/service/apple_mdm_ddm_test.go | Uses mysqltest DS setup + ExecAdhocSQL for DDM setup. |
| server/mdm/testing_utils/testing_utils.go | Replaces *testing.T with a local interface to avoid importing testing. |
| server/mdm/apple/apple_mdm_external_test.go | Uses mysqltest DS setup/truncation + ad-hoc SQL. |
| server/mdm/android/tests/testing_utils.go | Uses mysqltest.CreateNamedMySQLDS for Android integration DS. |
| server/mdm/android/tests/integration_software_test.go | Uses mysqltest.ExecAdhocSQL for DB assertions. |
| server/mdm/android/service/profiles_test.go | Uses mysqltest DS setup/truncation + ad-hoc SQL in helper assertions. |
| server/datastore/mysql/testing_utils_test.go | Removes replica helpers from test-only file; adjusts internal test accessors. |
| server/datastore/mysql/test_accessors.go | Adds exported “Test*” accessors/wrappers used by mysqltest. |
| server/datastore/mysql/replica_status.go | Adds production MasterStatus/ReplicaStatus methods for mysqltest usage. |
| server/datastore/mysql/mysqltest/mysqltest.go | New mysqltest package implementing DS setup + fixtures + helpers (imports testing). |
| pkg/mdm/mdmtest/apple.go | Switches to mysqltest.GenerateTestCertBytes. |
| ee/server/service/mdm_external_test.go | Switches to mysqltest cert + ABM asset generators. |
| ee/server/service/condaccess/config_test.go | Uses mysqltest DS setup/truncation. |
| ee/server/integrationtest/scim/scim_test.go | Uses mysqltest.TruncateTables for SCIM table cleanup. |
| ee/server/integrationtest/hostidentity/scep_rate_limit_test.go | Uses mysqltest.TruncateTables for SCEP tables cleanup. |
| ee/server/integrationtest/hostidentity/hostidentity_test.go | Uses mysqltest.TruncateTables for SCEP tables cleanup. |
| ee/server/integrationtest/hostidentity/hostidentity_requiresig_test.go | Uses mysqltest.TruncateTables for SCEP tables cleanup. |
| ee/server/integrationtest/condaccess/scep_rate_limit_test.go | Uses mysqltest.TruncateTables for conditional-access SCEP cleanup. |
| ee/server/integrationtest/condaccess/condaccess_test.go | Uses mysqltest.TruncateTables for conditional-access SCEP cleanup. |
| cmd/fleetctl/integrationtest/suite.go | Uses mysqltest.CreateNamedMySQLDS for fleetctl integration DS. |
| cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go | Uses mysqltest.ExecAdhocSQL for DB cleanup/assertions. |
| cmd/fleetctl/fleetctl/testing_utils/testing_utils.go | Uses mysqltest for ABM/cert fixture generation. |
| cmd/fleetctl/fleetctl/gitops_test.go | Uses mysqltest.GenerateTestCertBytes for MDM fixtures. |
| cmd/fleet/cron_test.go | Uses mysqltest DS setup + ABM test assets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
WalkthroughAdds server/datastore/mysql/mysqltest with extensive test helpers (datastore constructors including dummy/real replication, ad-hoc SQL execution, ABM/MDM asset and token seeding, activity adapters, table dump/SQL normalization). Adds replica-status APIs and exported test accessors in server/datastore/mysql. Migrates many tests and utilities across cmd/, server/, ee/, and pkg/ to use mysqltest.* helpers (CreateMySQLDS variants, TruncateTables, ExecAdhocSQL, GenerateTestCertBytes, SetTestABMAssets, GetAggregatedStats, NewTestActivityService, InsertWindowsProfileForTest, etc.) and introduces a TestingT interface and minor test adjustments. No production runtime behavior changes. Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/datastore/mysql/mysqltest/mysqltest.go`:
- Around line 153-163: The cancel variable can be nil when t is not *testing.T,
causing t.Cleanup(cancel) to panic; initialize cancel to a no-op (e.g., cancel =
func() {}) before the type-branch so t.Cleanup is always called with a valid
func(), then keep the existing assignments where ctx, cancel =
context.WithDeadline(...) or context.WithCancel(... ) inside the tt branch to
override the no-op when appropriate; ensure references to cancel, ctx,
t.Cleanup, context.WithDeadline and context.WithCancel are updated accordingly.
In `@server/datastore/mysql/replica_status.go`:
- Around line 27-30: The MasterStatus implementation currently calls
ds.writer(ctx).Query(stmt) and ignores the context; change this to use
QueryContext(ctx, stmt) so the database call honors cancellation/timeouts
(mirror the approach used in ReplicaStatus). Update the call in MasterStatus to
invoke QueryContext with the provided ctx and keep the existing error handling
(wrapping with ctxerr.Wrap) intact so behavior otherwise remains the same.
In `@server/datastore/mysql/test_accessors.go`:
- Around line 49-53: Remove the exported accessor Datastore.TestServerPrivateKey
to avoid exposing raw secrets; delete the TestServerPrivateKey method and
replace call sites (tests/fixtures) to use an operation-level helper (e.g.,
Datastore.TestEncrypt or another encryption helper) that performs
encryption/decryption without returning the private key. Ensure a
non-exported/internal helper exists on Datastore to perform the needed test
encryption (create/verify Datastore.testEncrypt that uses ds.serverPrivateKey
internally), update all references from TestServerPrivateKey to that helper, and
run tests to confirm no code path returns the raw key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4de400d4-46e4-4e5a-a2e6-8d09a373cc56
📒 Files selected for processing (75)
cmd/fleet/cron_test.gocmd/fleetctl/fleetctl/gitops_test.gocmd/fleetctl/fleetctl/testing_utils/testing_utils.gocmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.gocmd/fleetctl/integrationtest/suite.goee/server/integrationtest/condaccess/condaccess_test.goee/server/integrationtest/condaccess/scep_rate_limit_test.goee/server/integrationtest/hostidentity/hostidentity_requiresig_test.goee/server/integrationtest/hostidentity/hostidentity_test.goee/server/integrationtest/hostidentity/scep_rate_limit_test.goee/server/integrationtest/scim/scim_test.goee/server/service/condaccess/config_test.goee/server/service/mdm_external_test.gopkg/mdm/mdmtest/apple.goserver/datastore/mysql/mysqltest/mysqltest.goserver/datastore/mysql/replica_status.goserver/datastore/mysql/test_accessors.goserver/datastore/mysql/testing_utils_test.goserver/mdm/android/service/profiles_test.goserver/mdm/android/tests/integration_software_test.goserver/mdm/android/tests/testing_utils.goserver/mdm/apple/apple_mdm_external_test.goserver/mdm/testing_utils/testing_utils.goserver/service/apple_mdm_ddm_test.goserver/service/apple_mdm_test.goserver/service/async/async_bench_test.goserver/service/async/async_host_seen_test.goserver/service/async/async_label_test.goserver/service/async/async_policy_test.goserver/service/async/async_scheduled_query_stats_test.goserver/service/async/async_test.goserver/service/hosts_test.goserver/service/integration_android_certificate_templates_test.goserver/service/integration_android_software_test.goserver/service/integration_certificate_authorities_test.goserver/service/integration_core_test.goserver/service/integration_desktop_test.goserver/service/integration_enterprise_test.goserver/service/integration_enterprise_vulns_test.goserver/service/integration_install_test.goserver/service/integration_live_queries_test.goserver/service/integration_mdm_commands_test.goserver/service/integration_mdm_ddm_test.goserver/service/integration_mdm_dep_test.goserver/service/integration_mdm_lifecycle_test.goserver/service/integration_mdm_profiles_test.goserver/service/integration_mdm_release_worker_test.goserver/service/integration_mdm_setup_experience_test.goserver/service/integration_mdm_test.goserver/service/integration_software_titles_test.goserver/service/integration_sso_test.goserver/service/integration_vpp_install_test.goserver/service/integrationtest/android/android_test.goserver/service/integrationtest/android/suite.goserver/service/integrationtest/suite.goserver/service/labels_test.goserver/service/mdm_test.goserver/service/microsoft_mdm_integration_test.goserver/service/orbit_test.goserver/service/packs_test.goserver/service/queries_test.goserver/service/schedule/schedule_test.goserver/service/service_campaign_test.goserver/service/sessions_test.goserver/service/testing_client_test.goserver/service/users_test.goserver/vulnerabilities/macoffice/integration_analyzer_test.goserver/vulnerabilities/oval/analyzer_test.goserver/vulnerabilities/vulntest/gen_fixture_test.goserver/vulnerabilities/vulntest/vulnerability_scanning_test.goserver/worker/apple_mdm_test.goserver/worker/db_migrations_test.goserver/worker/macos_setup_assistant_test.goserver/worker/software_worker_test.goserver/worker/worker_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/datastore/mysql/mysqltest/mysqltest.go`:
- Around line 423-447: The loop in CreateMySQLDSWithReplica can return a closed
*mysql.Datastore (ds) if all numberOfAttempts fail; change the logic to track
success and fail the test when retries are exhausted instead of returning the
last closed ds: after each ReplicaStatus check, only set ds as the return value
when Replica_SQL_Running == "Yes" and break, otherwise Close() and continue;
after the loop assert that a successful ds was obtained (e.g.,
require.NotNil/require.FailNow with context) so the function never returns a
closed datastore when all attempts fail. Ensure you reference
CreateMySQLDSWithReplica, ds, ReplicaStatus and numberOfAttempts when updating
the control flow.
- Around line 907-909: The code currently ignores the error returned by
activity_bootstrap.New, which can hide misconfiguration; change the call in
mysqltest.go to check the returned error from activity_bootstrap.New (the svc,
err := activity_bootstrap.New(...)) and propagate or fail fast (e.g., return the
error up the test helper or call t.Fatalf in tests) instead of discarding it;
ensure the error is logged/returned so callers using svc see a clear failure
rather than operating on an invalid service instance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 47999669-c696-49e9-b1cc-05bf45cef6fb
📒 Files selected for processing (3)
server/datastore/mysql/mysqltest/mysqltest.goserver/datastore/mysql/replica_status.goserver/datastore/mysql/test_accessors.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/datastore/mysql/replica_status.go
Comment on lines
+423
to
+447
| func CreateMySQLDSWithReplica(t *testing.T, opts *testing_utils.DatastoreTestOptions) *mysql.Datastore { | ||
| if opts == nil { | ||
| opts = new(testing_utils.DatastoreTestOptions) | ||
| } | ||
| opts.RealReplica = true | ||
| const numberOfAttempts = 10 | ||
| var ds *mysql.Datastore | ||
| for attempt := 0; attempt < numberOfAttempts; { | ||
| attempt++ | ||
| ds = createMySQLDSWithOptions(t, opts) | ||
| status, err := ds.ReplicaStatus(context.Background()) | ||
| require.NoError(t, err) | ||
| if status["Replica_SQL_Running"] != "Yes" { | ||
| t.Logf("create replica attempt: %d replica status: %+v", attempt, status) | ||
| if lastErr, ok := status["Last_Error"]; ok && lastErr != "" { | ||
| t.Logf("replica not running after attempt %d; Last_Error: %s", attempt, lastErr) | ||
| } | ||
| ds.Close() | ||
| continue | ||
| } | ||
| break | ||
| } | ||
| require.NotNil(t, ds) | ||
| return ds | ||
| } |
Contributor
There was a problem hiding this comment.
CreateMySQLDSWithReplica returns a closed Datastore when all attempts fail.
If every one of the numberOfAttempts iterations sees Replica_SQL_Running != "Yes", the loop calls ds.Close() on each attempt and falls through without break. After the loop, ds still points at the last (now-closed) *mysql.Datastore, so require.NotNil(t, ds) passes and a closed datastore is handed back to the caller. Downstream test failures will then be confusing/non-localized rather than failing here at the source.
Track success explicitly (or fail when retries are exhausted) so a closed datastore is never returned.
🐛 Proposed fix
func CreateMySQLDSWithReplica(t *testing.T, opts *testing_utils.DatastoreTestOptions) *mysql.Datastore {
if opts == nil {
opts = new(testing_utils.DatastoreTestOptions)
}
opts.RealReplica = true
const numberOfAttempts = 10
var ds *mysql.Datastore
+ var ready bool
for attempt := 0; attempt < numberOfAttempts; {
attempt++
ds = createMySQLDSWithOptions(t, opts)
status, err := ds.ReplicaStatus(context.Background())
require.NoError(t, err)
if status["Replica_SQL_Running"] != "Yes" {
t.Logf("create replica attempt: %d replica status: %+v", attempt, status)
if lastErr, ok := status["Last_Error"]; ok && lastErr != "" {
t.Logf("replica not running after attempt %d; Last_Error: %s", attempt, lastErr)
}
ds.Close()
+ ds = nil
continue
}
+ ready = true
break
}
- require.NotNil(t, ds)
+ require.True(t, ready, "replica did not start after %d attempts", numberOfAttempts)
+ require.NotNil(t, ds)
return ds
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func CreateMySQLDSWithReplica(t *testing.T, opts *testing_utils.DatastoreTestOptions) *mysql.Datastore { | |
| if opts == nil { | |
| opts = new(testing_utils.DatastoreTestOptions) | |
| } | |
| opts.RealReplica = true | |
| const numberOfAttempts = 10 | |
| var ds *mysql.Datastore | |
| for attempt := 0; attempt < numberOfAttempts; { | |
| attempt++ | |
| ds = createMySQLDSWithOptions(t, opts) | |
| status, err := ds.ReplicaStatus(context.Background()) | |
| require.NoError(t, err) | |
| if status["Replica_SQL_Running"] != "Yes" { | |
| t.Logf("create replica attempt: %d replica status: %+v", attempt, status) | |
| if lastErr, ok := status["Last_Error"]; ok && lastErr != "" { | |
| t.Logf("replica not running after attempt %d; Last_Error: %s", attempt, lastErr) | |
| } | |
| ds.Close() | |
| continue | |
| } | |
| break | |
| } | |
| require.NotNil(t, ds) | |
| return ds | |
| } | |
| func CreateMySQLDSWithReplica(t *testing.T, opts *testing_utils.DatastoreTestOptions) *mysql.Datastore { | |
| if opts == nil { | |
| opts = new(testing_utils.DatastoreTestOptions) | |
| } | |
| opts.RealReplica = true | |
| const numberOfAttempts = 10 | |
| var ds *mysql.Datastore | |
| var ready bool | |
| for attempt := 0; attempt < numberOfAttempts; { | |
| attempt++ | |
| ds = createMySQLDSWithOptions(t, opts) | |
| status, err := ds.ReplicaStatus(context.Background()) | |
| require.NoError(t, err) | |
| if status["Replica_SQL_Running"] != "Yes" { | |
| t.Logf("create replica attempt: %d replica status: %+v", attempt, status) | |
| if lastErr, ok := status["Last_Error"]; ok && lastErr != "" { | |
| t.Logf("replica not running after attempt %d; Last_Error: %s", attempt, lastErr) | |
| } | |
| ds.Close() | |
| ds = nil | |
| continue | |
| } | |
| ready = true | |
| break | |
| } | |
| require.True(t, ready, "replica did not start after %d attempts", numberOfAttempts) | |
| require.NotNil(t, ds) | |
| return ds | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/datastore/mysql/mysqltest/mysqltest.go` around lines 423 - 447, The
loop in CreateMySQLDSWithReplica can return a closed *mysql.Datastore (ds) if
all numberOfAttempts fail; change the logic to track success and fail the test
when retries are exhausted instead of returning the last closed ds: after each
ReplicaStatus check, only set ds as the return value when Replica_SQL_Running ==
"Yes" and break, otherwise Close() and continue; after the loop assert that a
successful ds was obtained (e.g., require.NotNil/require.FailNow with context)
so the function never returns a closed datastore when all attempts fail. Ensure
you reference CreateMySQLDSWithReplica, ds, ReplicaStatus and numberOfAttempts
when updating the control flow.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/datastore/mysql/mysqltest/mysqltest.go`:
- Around line 285-302: The replication goroutine can block forever on the
unguarded send to out.replicationDone; change the send inside the goroutine to
mirror the caller by using a select that attempts to send on out.replicationDone
but also returns if ctx.Done() fires (i.e., replace the plain
out.replicationDone <- struct{}{} with a select { case out.replicationDone <-
struct{}{}: case <-ctx.Done(): } ), so the goroutine won’t leak when
opts.RunReplication races with ctx cancellation.
- Around line 628-656: CreateABMKeyCertIfNotExists currently can insert a
freshly generated cert/key that doesn't match an existing counterpart; change
the logic so the DB never ends up with a mismatched pair: call
GenerateTestABMAssets to get certPEM/keyPEM, then query both assets via
GetAllMDMConfigAssetsByName and if either is missing ensure you persist both
together (not just the missing one) — either no-op when exactly one exists or,
preferably, replace/insert both atomically (use InsertMDMConfigAssets with the
TestWriter transaction or an atomic upsert/delete+insert) so stored ABMKey and
ABMCert are always a matching pair; update CreateABMKeyCertIfNotExists to detect
the “one present” case and perform the atomic replace/insert of both assets.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 857f7331-b783-47f7-8155-264fec8e4584
📒 Files selected for processing (4)
cmd/fleetctl/fleetctl/gitops_test.goserver/datastore/mysql/mysqltest/mysqltest.goserver/service/integration_enterprise_test.goserver/service/integration_mdm_dep_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/fleetctl/fleetctl/gitops_test.go
- server/service/integration_mdm_dep_test.go
- server/service/integration_enterprise_test.go
Comment on lines
+285
to
+302
|
|
||
| out.replicationDone <- struct{}{} | ||
| t.Logf("replication step executed, next will consider updates since %s", last) | ||
|
|
||
| case <-ctx.Done(): | ||
| return | ||
| } | ||
| } | ||
| }() | ||
|
|
||
| opts.RunReplication = func(forceTables ...string) { | ||
| done := make(chan struct{}) | ||
| ch <- replicationRun{forceTables, done} | ||
| select { | ||
| case <-done: | ||
| case <-ctx.Done(): | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Replication goroutine can block forever on replicationDone if the caller bails via ctx.Done().
out.replicationDone <- struct{}{} is an unbuffered, unguarded send. opts.RunReplication already races the receive against ctx.Done(); if the deadline/cleanup fires first, the caller returns while the goroutine is still trying to send, and the goroutine is leaked for the rest of the process. Mirror the caller and select on ctx.Done() here too.
🛡️ Proposed fix
- out.replicationDone <- struct{}{}
- t.Logf("replication step executed, next will consider updates since %s", last)
+ select {
+ case out.replicationDone <- struct{}{}:
+ case <-ctx.Done():
+ return
+ }
+ t.Logf("replication step executed, next will consider updates since %s", last)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| out.replicationDone <- struct{}{} | |
| t.Logf("replication step executed, next will consider updates since %s", last) | |
| case <-ctx.Done(): | |
| return | |
| } | |
| } | |
| }() | |
| opts.RunReplication = func(forceTables ...string) { | |
| done := make(chan struct{}) | |
| ch <- replicationRun{forceTables, done} | |
| select { | |
| case <-done: | |
| case <-ctx.Done(): | |
| } | |
| } | |
| select { | |
| case out.replicationDone <- struct{}{}: | |
| case <-ctx.Done(): | |
| return | |
| } | |
| t.Logf("replication step executed, next will consider updates since %s", last) | |
| case <-ctx.Done(): | |
| return | |
| } | |
| } | |
| }() | |
| opts.RunReplication = func(forceTables ...string) { | |
| done := make(chan struct{}) | |
| ch <- replicationRun{forceTables, done} | |
| select { | |
| case <-done: | |
| case <-ctx.Done(): | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/datastore/mysql/mysqltest/mysqltest.go` around lines 285 - 302, The
replication goroutine can block forever on the unguarded send to
out.replicationDone; change the send inside the goroutine to mirror the caller
by using a select that attempts to send on out.replicationDone but also returns
if ctx.Done() fires (i.e., replace the plain out.replicationDone <- struct{}{}
with a select { case out.replicationDone <- struct{}{}: case <-ctx.Done(): } ),
so the goroutine won’t leak when opts.RunReplication races with ctx
cancellation.
Comment on lines
+628
to
+656
| // CreateABMKeyCertIfNotExists ensures the ABM key/cert mdm_config_assets rows | ||
| // exist (generating them if missing). | ||
| func CreateABMKeyCertIfNotExists(t testing.TB, ds *mysql.Datastore) { | ||
| certPEM, keyPEM, _, err := GenerateTestABMAssets(t) | ||
| require.NoError(t, err) | ||
| var assets []fleet.MDMConfigAsset | ||
| _, err = ds.GetAllMDMConfigAssetsByName(context.Background(), []fleet.MDMAssetName{ | ||
| fleet.MDMAssetABMKey, | ||
| }, nil) | ||
| if err != nil { | ||
| var nfe fleet.NotFoundError | ||
| require.ErrorAs(t, err, &nfe) | ||
| assets = append(assets, fleet.MDMConfigAsset{Name: fleet.MDMAssetABMKey, Value: keyPEM}) | ||
| } | ||
|
|
||
| _, err = ds.GetAllMDMConfigAssetsByName(context.Background(), []fleet.MDMAssetName{ | ||
| fleet.MDMAssetABMCert, | ||
| }, nil) | ||
| if err != nil { | ||
| var nfe fleet.NotFoundError | ||
| require.ErrorAs(t, err, &nfe) | ||
| assets = append(assets, fleet.MDMConfigAsset{Name: fleet.MDMAssetABMCert, Value: certPEM}) | ||
| } | ||
|
|
||
| if len(assets) != 0 { | ||
| err = ds.InsertMDMConfigAssets(context.Background(), assets, ds.TestWriter(context.Background())) | ||
| require.NoError(t, err) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
CreateABMKeyCertIfNotExists can persist a mismatched cert/key pair.
GenerateTestABMAssets returns a freshly generated, paired cert and key. The function then independently checks for MDMAssetABMKey and MDMAssetABMCert and only inserts the missing one(s). If exactly one of the two assets already exists in the DB, this inserts the freshly generated counterpart, which is unrelated to the existing one. Subsequent test code that decodes the token with the stored cert/key pair will silently use mismatched halves.
Either bail out / no-op when only one half is present, or replace both atomically so they remain paired.
🛡️ Proposed fix — insert both halves when either is missing
func CreateABMKeyCertIfNotExists(t testing.TB, ds *mysql.Datastore) {
certPEM, keyPEM, _, err := GenerateTestABMAssets(t)
require.NoError(t, err)
- var assets []fleet.MDMConfigAsset
- _, err = ds.GetAllMDMConfigAssetsByName(context.Background(), []fleet.MDMAssetName{
- fleet.MDMAssetABMKey,
- }, nil)
- if err != nil {
- var nfe fleet.NotFoundError
- require.ErrorAs(t, err, &nfe)
- assets = append(assets, fleet.MDMConfigAsset{Name: fleet.MDMAssetABMKey, Value: keyPEM})
- }
-
- _, err = ds.GetAllMDMConfigAssetsByName(context.Background(), []fleet.MDMAssetName{
- fleet.MDMAssetABMCert,
- }, nil)
- if err != nil {
- var nfe fleet.NotFoundError
- require.ErrorAs(t, err, &nfe)
- assets = append(assets, fleet.MDMConfigAsset{Name: fleet.MDMAssetABMCert, Value: certPEM})
- }
-
- if len(assets) != 0 {
- err = ds.InsertMDMConfigAssets(context.Background(), assets, ds.TestWriter(context.Background()))
- require.NoError(t, err)
- }
+ existing, err := ds.GetAllMDMConfigAssetsByName(context.Background(), []fleet.MDMAssetName{
+ fleet.MDMAssetABMKey, fleet.MDMAssetABMCert,
+ }, nil)
+ if err != nil {
+ var nfe fleet.NotFoundError
+ require.ErrorAs(t, err, &nfe)
+ }
+ _, hasKey := existing[fleet.MDMAssetABMKey]
+ _, hasCert := existing[fleet.MDMAssetABMCert]
+ if hasKey && hasCert {
+ return
+ }
+ require.False(t, hasKey != hasCert,
+ "existing ABM key/cert is partial; refusing to insert an unmatched counterpart")
+ err = ds.InsertMDMConfigAssets(context.Background(), []fleet.MDMConfigAsset{
+ {Name: fleet.MDMAssetABMKey, Value: keyPEM},
+ {Name: fleet.MDMAssetABMCert, Value: certPEM},
+ }, ds.TestWriter(context.Background()))
+ require.NoError(t, err)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/datastore/mysql/mysqltest/mysqltest.go` around lines 628 - 656,
CreateABMKeyCertIfNotExists currently can insert a freshly generated cert/key
that doesn't match an existing counterpart; change the logic so the DB never
ends up with a mismatched pair: call GenerateTestABMAssets to get
certPEM/keyPEM, then query both assets via GetAllMDMConfigAssetsByName and if
either is missing ensure you persist both together (not just the missing one) —
either no-op when exactly one exists or, preferably, replace/insert both
atomically (use InsertMDMConfigAssets with the TestWriter transaction or an
atomic upsert/delete+insert) so stored ABMKey and ABMCert are always a matching
pair; update CreateABMKeyCertIfNotExists to detect the “one present” case and
perform the atomic replace/insert of both assets.
getvictor
approved these changes
May 14, 2026
Member
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
Looks good.
Did you consider using //go:build testing to exclude specific files from the compile? Like test_accessors.go?
Member
Author
Good idea. Will take a look at doing something like this. Also, once all is moved I plan to add a CI check that builds fleet and fleetctl and makes sure "testing" is not a dependency in the binaries. |
lucasmrod
deleted the
remove-more-testing-utils-from-production-binaries-round-4
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #45220 (one of several PRs to achieve removing "testing" package as dependency in production binary)
Testing
Summary by CodeRabbit
Tests
Chores