Replace test set-style map[string]bool usage with map[string]struct{}#37257
Conversation
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
map[string]bool usage with map[string]struct{}
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This PR standardizes test-only “set” usage across the codebase by replacing map[string]bool patterns with idiomatic map[string]struct{} and updating related membership checks and assertions. It also introduces a small compatibility converter for cases where existing APIs still accept map[string]bool.
Changes:
- Converted many test/fuzz set-like maps from
map[string]booltomap[string]struct{}and updated write patterns tom[k] = struct{}{}. - Reworked membership checks to use key-existence (
_, ok := m[k]) and updated assertions toassert.Contains/assert.NotContainswhere appropriate. - Added
stringSetToBoolMapinpkg/cli/engine_secrets.goto bridge tests that still callmap[string]boolAPIs.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/tools_types_test.go | Uses map[string]struct{} + delete to verify expected tool names without ordering sensitivity. |
| pkg/workflow/maintenance_workflow_test.go | Converts result set to map[string]struct{} and uses existence checks for membership assertions. |
| pkg/workflow/known_needs_expressions_test.go | Replaces boolean reads with assert.Contains / assert.NotContains against a struct{} set. |
| pkg/workflow/github_toolsets_test.go | Migrates multiple set-like maps to map[string]struct{} with existence checks. |
| pkg/workflow/git_commands_test.go | Switches editing-tool membership tracking to map[string]struct{} and updates checks accordingly. |
| pkg/workflow/expression_extraction_fuzz_test.go | Uses map[string]struct{} to detect duplicates in fuzz outputs. |
| pkg/workflow/ecosystem_domains_test.go | Uses a struct{} set to detect duplicate domains. |
| pkg/workflow/domains_test.go | Replaces domain membership bool-map with existence semantics on a struct{} set. |
| pkg/workflow/domains_blocked_test.go | Replaces domain accumulation map with map[string]struct{}. |
| pkg/workflow/custom_job_condition_test.go | Converts expected/actual result comparisons to struct{} sets. |
| pkg/workflow/compiler_jobs_test.go | Uses map[string]struct{} to validate included/excluded job sets. |
| pkg/workflow/claude_engine_tools_test.go | Converts expected/actual tool collections into struct{} sets and compares via existence checks. |
| pkg/workflow/bash_merge_test.go | Uses map[string]struct{} for order-independent membership validation. |
| pkg/workflow/agentic_engine_test.go | Uses assert.NotContains with a struct{} set to detect duplicates. |
| pkg/sliceutil/sliceutil_test.go | Updates the MapKeys test to use a map[string]struct{} input. |
| pkg/parser/schema_deprecated_test.go | Uses struct{} sets for found fields/paths membership checks. |
| pkg/parser/mcp_test.go | Uses map[string]struct{} for order-independent args comparison via reflect.DeepEqual. |
| pkg/cli/tokens_bootstrap_test.go | Uses assert.Contains on a struct{} set of engine secret names. |
| pkg/cli/secrets_test.go | Converts expected/actual secret name sets to map[string]struct{} with existence checks. |
| pkg/cli/packages_test.go | Uses map[string]struct{} for the seen-set in tests and converts via stringSetToBoolMap for bool-map APIs. |
| pkg/cli/logs_report_test.go | Represents allowed/denied domain sets as map[string]struct{} and converts for bool-map API usage. |
| pkg/cli/engine_secrets.go | Adds stringSetToBoolMap converter helper for tests interacting with bool-map APIs. |
| pkg/cli/engine_secrets_test.go | Updates existing-secrets sets to map[string]struct{} and converts when calling bool-map APIs. |
| pkg/cli/dependency_graph_test.go | Uses map[string]struct{} sets for membership assertions on affected workflows. |
| pkg/cli/copilot_events_jsonl_test.go | Uses assert.Contains on a struct{} set of tool call names. |
| pkg/cli/add_interactive_schedule_test.go | Uses map[string]struct{} for schedule option membership checks with assert.Contains. |
| pkg/actionpins/actionpins_internal_test.go | Avoids map[string]bool as a test set while still populating the Warnings map[string]bool API. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 27/27 changed files
- Comments generated: 2
| func stringSetToBoolMap(set map[string]struct{}) map[string]bool { | ||
| if len(set) == 0 { | ||
| return map[string]bool{} | ||
| } | ||
|
|
||
| result := make(map[string]bool, len(set)) | ||
| for item := range set { | ||
| result[item] = true | ||
| } | ||
| return result | ||
| } |
| func stringSetToBoolMap(set map[string]struct{}) map[string]bool { | ||
| if len(set) == 0 { | ||
| return map[string]bool{} | ||
| } | ||
|
|
||
| result := make(map[string]bool, len(set)) | ||
| for item := range set { | ||
| result[item] = true | ||
| } | ||
| return result | ||
| } |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 94/100 — Excellent
📊 Metrics & Test Classification (32 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §27065144413
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 94/100. Test quality is excellent — 0% of modified tests are implementation tests (threshold: 30%). All 32 modified tests preserve their behavioral contracts through this idiomatic map[string]struct{} refactoring. No coding-guideline violations detected.
There was a problem hiding this comment.
Two maintainability issues with the engine_secrets.go lint remediation strategy; everything else (CutPrefix refactor, YAML tag fix, lock-file recompile, test-local set conversions) looks correct.
🔍 Findings
Test helper leaked into production source
stringSetToBoolMap is only called from _test.go files but lives in engine_secrets.go. It should be moved to engine_secrets_test.go. Details in the inline comment at line 177.
Lint fix direction is inverted
Test code was changed to use map[string]struct{}{} while the production API (getMissingRequiredSecrets, GetEngineSecretNameAndValue, EngineSecretConfig.ExistingSecrets, etc.) still uses map[string]bool. This leaves the actual linting violation in production code untouched and adds a redundant allocation at every test call site. Details and alternatives in the inline comment at line 159.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 616.5K · 3.92 AIC · ⌖ 13.2 AIC
Comments that could not be inline-anchored
pkg/cli/engine_secrets.go:177
Test-only helper placed in production code: stringSetToBoolMap is only called from test files but lives in the production package, adding dead weight to the binary and muddying the package API.
<details>
<summary>💡 Suggested fix</summary>
Move the function body verbatim into engine_secrets_test.go (or a shared testhelpers_test.go). Since it is in the same package (package cli), the test files can access all unexported identifiers without it being in the production source.
…
</details>
<details><summary>pkg/cli/engine_secrets.go:159</summary>
**Lint fix applied in the wrong direction**: the `seenmapbool` violation exists in both test and production code, but only the test side was changed. `getMissingRequiredSecrets`, `GetEngineSecretNameAndValue`, `displayMissingSecrets`, `displaySecretsSummaryTable`, and the `ExistingSecrets` struct field all still take/hold `map[string]bool` used purely as a membership set.
<details>
<summary>💡 Impact and suggested approach</summary>
The current approach:
1. Changed test-side maps to `map[stri…
</details>
Daily linting flagged widespread test-only set patterns implemented as
map[string]bool, which are non-idiomatic and allocate unnecessary values. This change converts the flagged set usages tomap[string]struct{}and updates related write/read patterns without changing test intent.Scope: test set patterns
map[string]booltomap[string]struct{}.m[k] = truetom[k] = struct{}{}.Membership checks and assertions
_, ok := m[k]) where direct boolean reads were used.assert.Contains/assert.NotContainsfor set membership checks).Compatibility where APIs still require
map[string]boolpkg/cli/engine_secrets.go(stringSetToBoolMap) and used it in affected tests that pass set data into bool-map APIs, keeping production behavior unchanged while allowing idiomatic set representation in test setup.