tests: make cleanup idempotent, remove truncate path, add test metric…#124
Open
coolwater wants to merge 5 commits into
Open
tests: make cleanup idempotent, remove truncate path, add test metric…#124coolwater wants to merge 5 commits into
coolwater wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors test cleanup to avoid Cassandra TRUNCATE latency by deleting per-row keys, switches many tests to t.Cleanup(...) for more idempotent teardown, adds a go test -json runner that emits a per-package metrics summary, and tweaks a couple of query services to treat “not found” as an empty/idempotent case.
Changes:
- Replace TRUNCATE-based test cleanup helpers with per-row delete-by-key logic and move several tests to
t.Cleanup(...). - Add
contrib/scripts/run_tests_with_summary.shand wiremake testto use it for JSON logs + metrics summary output. - Adjust query services to treat “not found” as a valid empty/idempotent scenario (percent filter and IP filter delete).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Routes make test through the new test runner script. |
| contrib/scripts/run_tests_with_summary.sh | Runs go test -json, writes logs, and generates a metrics summary via an embedded Python parser. |
| adminapi/telemetry/telemetry_v2_rule_service_test.go | Replaces package-wide cleanup patterns with t.Cleanup and per-entity deletes for test data. |
| adminapi/telemetry/telemetry_profile_handler_test.go | Adds a test watchdog goroutine and changes TRUNCATE logic to per-key deletes. |
| adminapi/rfc/feature/feature_test_helpers_test.go | Updates cleanup helper to delete rows by key rather than TRUNCATE. |
| adminapi/rfc/feature/feature_handler_test.go | Adds test watchdog and replaces DB-wide cleanup with per-entity t.Cleanup deletes. |
| adminapi/queries/queries_test.go | Adds test watchdog and changes TRUNCATE cleanup to per-key deletes. |
| adminapi/queries/percentage_bean_service_test.go | Updates tests to delete only created rows and uses unique IDs in some cases. |
| adminapi/queries/percent_filter_service.go | Treats “not found” from env/model rule lookup as an empty ruleset. |
| adminapi/queries/ips_filter_service.go | Makes delete idempotent by treating “not found” as 204 No Content. |
| adminapi/dcm/logupload_settings_handler_test.go | Removes global cleanup calls in favor of targeted t.Cleanup deletes. |
| adminapi/dcm/dcmformula_test.go | Adds watchdog + per-key cleanup; updates formula IDs for uniqueness; adjusts assertions in sort-by-priority test. |
Comments suppressed due to low confidence (4)
adminapi/telemetry/telemetry_v2_rule_service_test.go:342
- This test also registers both
t.Cleanup(DeleteTelemetryEntities)and a per-row delete cleanup for the same table. Consider consolidating to one cleanup strategy to avoid redundant DB work and potential noisy delete errors.
func TestFindByContext_ApplicationTypeFilter(t *testing.T) {
DeleteTelemetryEntities()
t.Cleanup(DeleteTelemetryEntities)
rule1 := createTestTelemetryTwoRule("Rule1", "stb", []string{})
rule2 := createTestTelemetryTwoRule("Rule2", "rdkcloud", []string{})
logupload.SetOneTelemetryTwoRule(rule1.ID, rule1)
logupload.SetOneTelemetryTwoRule(rule2.ID, rule2)
t.Cleanup(func() {
DeleteOneFromDao(ds.TABLE_TELEMETRY_TWO_RULES, rule1.ID)
DeleteOneFromDao(ds.TABLE_TELEMETRY_TWO_RULES, rule2.ID)
})
adminapi/queries/queries_test.go:141
truncateTablealways returnsnileven when individual deletes fail (delErris only printed). Callers that check the returned error (e.g.,DeleteAllEntities) will never detect cleanup failures. Consider accumulating the first/last delete error and returning it (or returning a combined error) instead of always returning nil.
if delErr := dao.DeleteOne(tableName, keyStr); delErr != nil {
fmt.Printf("failed to delete %s from %s: %v\n", keyStr, tableName, delErr)
}
}
return nil
adminapi/dcm/dcmformula_test.go:960
truncateTablealways returnsnileven when individual deletes fail (delErris only printed). Any callers checking the returned error won’t detect cleanup failures. Consider returning a non-nil error when any delete fails (and/or whenGetKeysfails for reasons other than "empty/not found").
if delErr := dao.DeleteOne(tableName, keyStr); delErr != nil {
fmt.Printf("failed to delete %s from %s: %v\n", keyStr, tableName, delErr)
}
}
return nil
}
adminapi/rfc/feature/feature_test_helpers_test.go:125
truncateTablealways returnsnileven when row deletes fail (delErris only printed). This makesDeleteAllEntitiesunable to detect/report cleanup failures. Consider returning an error if any delete fails instead of always returning nil.
if delErr := dao.DeleteOne(tableName, keyStr); delErr != nil {
fmt.Printf("failed to delete %s from %s: %v\n", keyStr, tableName, delErr)
}
}
return nil
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+68
to
+71
| if not test and action in ("pass", "fail"): | ||
| stats[pkg]["pkg_status"] = action | ||
| if "Elapsed" in evt: | ||
| stats[pkg]["elapsed"] = float(evt["Elapsed"]) |
Comment on lines
+24
to
+25
| python3 - "${JSON_LOG}" <<'PY' | tee "${SUMMARY_LOG}" | ||
| import json |
Comment on lines
+99
to
+100
| stopWatchdog := startTestWatchdog("adminapi/telemetry") | ||
| defer stopWatchdog() |
Comment on lines
+58
to
+61
| func TestMain(m *testing.M) { | ||
| stopWatchdog := startTestWatchdog("adminapi/rfc/feature") | ||
| defer stopWatchdog() | ||
|
|
Comment on lines
143
to
147
| func TestMain(m *testing.M) { | ||
| fmt.Printf("in TestMain\n") | ||
| stopWatchdog := startTestWatchdog("adminapi/queries") | ||
| defer stopWatchdog() | ||
|
|
Comment on lines
+445
to
+450
| dao := db.GetCachedSimpleDao() | ||
| keys, err := dao.GetKeys(tableName) | ||
| if err != nil { | ||
| // table may be empty or not yet exist; not an error | ||
| return nil | ||
| } |
Comment on lines
+121
to
+126
| dao := db.GetCachedSimpleDao() | ||
| keys, err := dao.GetKeys(tableName) | ||
| if err != nil { | ||
| // table may be empty or not yet exist; not an error | ||
| return nil | ||
| } |
Comment on lines
+939
to
+944
| dao := db.GetCachedSimpleDao() | ||
| keys, err := dao.GetKeys(tableName) | ||
| if err != nil { | ||
| // table may be empty or not yet exist; not an error | ||
| return nil | ||
| } |
Comment on lines
+104
to
+109
| dao := db.GetCachedSimpleDao() | ||
| keys, err := dao.GetKeys(tableName) | ||
| if err != nil { | ||
| // table may be empty or not yet exist; not an error | ||
| return nil | ||
| } |
Comment on lines
143
to
166
| // TestGetLogUploadSettingsHandler_FilterByApplicationType tests filtering by application type | ||
| func TestGetLogUploadSettingsHandler_FilterByApplicationType(t *testing.T) { | ||
| SkipIfMockDatabase(t) // Integration test | ||
| DeleteAllEntities() | ||
| defer DeleteAllEntities() | ||
|
|
||
| // Create formulas for different application types | ||
| formulaStb := createFormula("TEST_MODEL_STB", 1) | ||
| saveFormula(formulaStb, t) | ||
|
|
||
| settingsStb := &logupload.LogUploadSettings{ | ||
| ID: formulaStb.ID, | ||
| Name: "STB Settings", | ||
| UploadRepositoryID: "test-repo", | ||
| ApplicationType: "stb", | ||
| Schedule: logupload.Schedule{ | ||
| Type: "CronExpression", | ||
| Expression: "0 0 * * *", | ||
| TimeZone: "UTC", | ||
| }, | ||
| } | ||
| CreateLogUploadSettings(settingsStb, "stb") | ||
|
|
||
| t.Cleanup(func() { | ||
| deleteOneFromDao(ds.TABLE_DCM_RULE, formulaStb.ID) | ||
| deleteOneFromDao(ds.TABLE_LOG_UPLOAD_SETTINGS, formulaStb.ID) | ||
| }) | ||
| req := httptest.NewRequest("GET", "/xconfAdminService/dcm/logUploadSettings", nil) |
Comment on lines
+46
to
+58
| func initMockGroupService() { | ||
| mockGroupServiceOnce.Do(func() { | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| groups := &proto_generated.XdasHashes{Fields: map[string]string{}} | ||
| data, _ := proto.Marshal(groups) | ||
| w.Header().Set("Content-Type", "application/x-protobuf") | ||
| w.WriteHeader(http.StatusOK) | ||
| _, _ = w.Write(data) | ||
| })) | ||
|
|
||
| mockGroupServiceURL = server.URL | ||
| mockGroupServiceHTTP = server.Client() | ||
| }) |
| mockGroupServiceOnce.Do(func() { | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| groups := &proto_generated.XdasHashes{Fields: map[string]string{}} | ||
| data, _ := proto.Marshal(groups) |
Comment on lines
143
to
147
| func TestMain(m *testing.M) { | ||
| fmt.Printf("in TestMain\n") | ||
| stopWatchdog := startTestWatchdog("adminapi/queries") | ||
| defer stopWatchdog() | ||
|
|
Comment on lines
120
to
141
| func truncateTable(tableName string) error { | ||
| dbClient := db.GetDatabaseClient() | ||
| cassandraClient, ok := dbClient.(*db.CassandraClient) | ||
| if ok { | ||
| return cassandraClient.DeleteAllXconfData(tableName) | ||
| dao := db.GetCachedSimpleDao() | ||
| keys, err := dao.GetKeys(tableName) | ||
| if err != nil { | ||
| // table may be empty or not yet exist; not an error | ||
| return nil | ||
| } | ||
| for _, key := range keys { | ||
| var keyStr string | ||
| switch k := key.(type) { | ||
| case string: | ||
| keyStr = k | ||
| case []byte: | ||
| keyStr = string(k) | ||
| default: | ||
| keyStr = fmt.Sprint(k) | ||
| } | ||
| if delErr := dao.DeleteOne(tableName, keyStr); delErr != nil { | ||
| fmt.Printf("failed to delete %s from %s: %v\n", keyStr, tableName, delErr) | ||
| } | ||
| } | ||
| return nil |
Comment on lines
113
to
134
| func truncateTable(tableName string) error { | ||
| dbClient := db.GetDatabaseClient() | ||
| cassandraClient, ok := dbClient.(*db.CassandraClient) | ||
| if ok { | ||
| return cassandraClient.DeleteAllXconfData(tableName) | ||
| dao := db.GetCachedSimpleDao() | ||
| keys, err := dao.GetKeys(tableName) | ||
| if err != nil { | ||
| // table may be empty or not yet exist; not an error | ||
| return nil | ||
| } | ||
| for _, key := range keys { | ||
| var keyStr string | ||
| switch k := key.(type) { | ||
| case string: | ||
| keyStr = k | ||
| case []byte: | ||
| keyStr = string(k) | ||
| default: | ||
| keyStr = fmt.Sprint(k) | ||
| } | ||
| if delErr := dao.DeleteOne(tableName, keyStr); delErr != nil { | ||
| fmt.Printf("failed to delete %s from %s: %v\n", keyStr, tableName, delErr) | ||
| } | ||
| } | ||
| return nil |
|
|
||
| test: | ||
| ulimit -n 10000 ; go test ./... -p 1 -parallel 1 -cover -count=1 -timeout=45m | ||
| bash contrib/scripts/run_tests_with_summary.sh |
Comment on lines
+19
to
+26
| set +e | ||
| go test ./... -json -p 1 -parallel 1 -cover -count=1 -timeout=45m | tee "${JSON_LOG}" | ||
| TEST_EXIT=${PIPESTATUS[0]} | ||
| set -e | ||
|
|
||
| python3 - "${JSON_LOG}" <<'PY' | tee "${SUMMARY_LOG}" | ||
| import json | ||
| import sys |
Comment on lines
80
to
88
| firmwareRules, err := corefw.GetEnvModelFirmwareRules(applicationType) | ||
| if err != nil { | ||
| if strings.Contains(strings.ToLower(err.Error()), "not found") { | ||
| // Empty rules set is valid; return default percent filter. | ||
| return percentFilterValue, nil | ||
| } | ||
| log.Error(fmt.Sprintf("GetPercentFilter: %v", err)) | ||
| return nil, err | ||
| } |
Comment on lines
66
to
74
| func DeleteIpsFilter(name string, applicationType string) *xwhttp.ResponseEntity { | ||
| ipFilter, err := coreef.IpFilterByName(name, applicationType) | ||
| if err != nil { | ||
| if strings.Contains(strings.ToLower(err.Error()), "not found") { | ||
| // Deleting a missing filter is treated as an idempotent no-op. | ||
| return xwhttp.NewResponseEntity(http.StatusNoContent, nil, nil) | ||
| } | ||
| return xwhttp.NewResponseEntity(http.StatusInternalServerError, err, nil) | ||
| } |
…t filter services; update tests to expect error responses
…ult value outcomes when no rules exist
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (2)
adminapi/queries/queries_test.go:141
- Errors from dao.DeleteOne are only printed and then ignored, and truncateTable still returns nil. If cleanup fails, subsequent tests may read stale data and become flaky. Consider collecting the first (or all) delete errors and returning a non-nil error so the test suite can fail fast.
if delErr := dao.DeleteOne(tableName, keyStr); delErr != nil {
fmt.Printf("failed to delete %s from %s: %v\n", keyStr, tableName, delErr)
}
}
return nil
adminapi/rfc/feature/feature_test_helpers_test.go:134
- Cleanup errors from dao.DeleteOne are only printed and then ignored, and truncateTable still returns nil. If deletions fail, subsequent tests may be affected by leftover rows. Consider returning an error (or failing the test) when any row deletion fails.
if delErr := dao.DeleteOne(tableName, keyStr); delErr != nil {
fmt.Printf("failed to delete %s from %s: %v\n", keyStr, tableName, delErr)
}
}
return nil
| mockGroupServiceOnce.Do(func() { | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| groups := &proto_generated.XdasHashes{Fields: map[string]string{}} | ||
| data, _ := proto.Marshal(groups) |
Comment on lines
143
to
147
| func TestMain(m *testing.M) { | ||
| fmt.Printf("in TestMain\n") | ||
| stopWatchdog := startTestWatchdog("adminapi/queries") | ||
| defer stopWatchdog() | ||
|
|
Comment on lines
+20
to
+21
| go test ./... -json -p 1 -parallel 1 -cover -count=1 -timeout=45m | tee "${JSON_LOG}" | ||
| TEST_EXIT=${PIPESTATUS[0]} |
|
|
||
| test: | ||
| ulimit -n 10000 ; go test ./... -p 1 -parallel 1 -cover -count=1 -timeout=45m | ||
| bash contrib/scripts/run_tests_with_summary.sh |
Comment on lines
+24
to
+27
| python3 - "${JSON_LOG}" <<'PY' | tee "${SUMMARY_LOG}" | ||
| import json | ||
| import sys | ||
| from collections import defaultdict |
Comment on lines
+48
to
+52
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| groups := &proto_generated.XdasHashes{Fields: map[string]string{}} | ||
| data, _ := proto.Marshal(groups) | ||
| w.Header().Set("Content-Type", "application/x-protobuf") | ||
| w.WriteHeader(http.StatusOK) |
Comment on lines
+122
to
+126
| keys, err := dao.GetKeys(tableName) | ||
| if err != nil { | ||
| // table may be empty or not yet exist; not an error | ||
| return nil | ||
| } |
Comment on lines
+79
to
+86
| if err != nil { | ||
| t.Logf("GetPercentFilter returned error as allowed: %v", err) | ||
| return | ||
| } | ||
| if pf == nil { | ||
| t.Errorf("Expected non-nil PercentFilterValue or error, got nil without error") | ||
| return | ||
| } |
Comment on lines
+115
to
+119
| keys, err := dao.GetKeys(tableName) | ||
| if err != nil { | ||
| // table may be empty or not yet exist; not an error | ||
| return nil | ||
| } |
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.
Replaced table truncation cleanup with per-row delete-by-key in test helpers (real DB path).
Removed dual cleanup patterns (DeleteAllEntities + defer DeleteAllEntities) and switched to test-scoped t.Cleanup deletes.
Made tests idempotent/parallel-safe by deleting only objects created by each test and using unique IDs where needed.
Kept real DB behavior (no enabling of USE_MOCK_DB in test runner).
Fixed empty/not-found behavior expected by tests in queries services.
TOTAL: 3230, PASSED: 3143, FAILED: 0 , SKIPPED: 87
Time: 1m 19s
Cassandra: 5.0.6
Hardware: M3 MacBook Pro