Skip to content

[test-improver] Improve tests for syncutil.Registry.GetOrCreate - #13603

Merged
lpcox merged 4 commits into
mainfrom
test-improver/syncutil-registry-getorcreate-coverage-5f7eb7aecc26ae25
Sep 22, 2026
Merged

lpcox merged 4 commits into
mainfrom
test-improver/syncutil-registry-getorcreate-coverage-5f7eb7aecc26ae25

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

File analyzed

internal/syncutil/registry_test.go / internal/syncutil/registry.go

Improvement made

Registry.GetOrCreate had 91.7% coverage: the write-locked double-check branch (lines 54-56 — hit when a goroutine loses the write-lock race but finds the key already populated by the winner) was only reachable probabilistically via the existing 64-goroutine stress test (TestRegistryGetOrCreateManyGoroutinesForcesDoubleCheck), so it wasn't guaranteed to fire on every CI run.

Added internal/syncutil/registry_internal_test.go (whitebox package syncutil, matching the existing ttl_cache_test.go pattern) with TestRegistryGetOrCreateDoubleCheckPreventsRedundantCreate, a deterministic test that:

  1. Holds registry.mu before starting two goroutines so both are guaranteed to block on mu.RLock().
  2. Releases the lock so both goroutines observe a cache miss and race for the write lock.
  3. Blocks the winner's create() call on a channel so the loser is forced to queue behind the write lock, guaranteeing it hits the double-check on every run.

This mirrors the analogous MapGetOrCreate double-check test already present in cache_test.go, reusing testify (assert/require) and timeout-guarded channel selects for stability (no flakiness across repeated -race runs).

The whitebox file is needed because Registry.mu is unexported — the external syncutil_test package (used by the other Registry tests) has no way to hold the lock deterministically.

Coverage

  • internal/syncutil package: 98.9% → 100.0%
  • Registry.GetOrCreate: 91.7% → 100.0%

Test output

$ go test -count=5 -race ./internal/syncutil/...
ok  	github.com/github/gh-aw-mcpg/internal/syncutil	2.043s

$ go vet ./internal/syncutil/...
(clean)

$ gofmt -l internal/syncutil/
(clean)

No existing tests were modified; all previously passing tests continue to pass.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • thishostdoesnotexist12345.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "thishostdoesnotexist12345.com"

See Network Configuration for more information.

Generated by Test Improver · copilot · auto · 246.5 AIC · ⊞ 8.4K ·

internal/syncutil/registry.go's GetOrCreate had 91.7% coverage: the
write-locked double-check branch (line 54-56, hit when a second goroutine
loses the write-lock race but finds the key already populated) was only
reachable probabilistically via the existing 64-goroutine stress test, and
was not guaranteed to fire on every CI run.

Add registry_internal_test.go (package syncutil, matching the existing
ttl_cache_test.go whitebox pattern) with a deterministic test that holds
registry.mu before starting two goroutines, releases it once both are
blocked on the read lock, and coordinates the write-lock winner's create()
call via channels so the loser is guaranteed to queue behind the write lock
and hit the double-check on every run. This mirrors the analogous
MapGetOrCreate double-check test already present in cache_test.go.

Coverage: internal/syncutil 98.9% -> 100.0%; GetOrCreate 91.7% -> 100.0%.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@lpcox
lpcox marked this pull request as ready for review September 22, 2026 14:51
Copilot AI balanced review requested due to automatic review settings September 22, 2026 14:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The synchronization does not guarantee both goroutines miss the initial lookup, so the target branch may remain uncovered.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Medium severity · 1 Low severity

Open (2)
What changed in this PR

Adds a white-box concurrency test intended to deterministically cover Registry.GetOrCreate’s double-check branch.

Changes:

  • Adds coordinated concurrent GetOrCreate calls.
  • Verifies single creation and cached results.
File Description
internal/​syncutil/​registry_internal_test.go Adds the concurrency test.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +45 to +47
<-start
callStarted <- struct{}{}
v := registry.GetOrCreate("key", func() int {
Comment on lines +25 to +26
func TestRegistryGetOrCreateDoubleCheckPreventsRedundantCreate(t *testing.T) {
registry := NewRegistry[string, int]()
@lpcox

lpcox commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator

@copilot address review feedback

Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
@github-actions

This comment has been minimized.

Copilot AI and others added 2 commits September 22, 2026 15:05
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>

Copilot AI commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

@copilot address review feedback

Addressed in 31f7118. The test now verifies both initial reads miss before releasing either worker into the write-locked double-check, uses bound testify asserters, and has timeout-safe cleanup. Repeated -race runs, 100% syncutil coverage, make agent-finished, code review, and CodeQL all pass.

@github-actions

Copy link
Copy Markdown
Contributor Author

🔒 mcpg Read-Only Stress — default

Surface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Isolation runtime: default (normal container isolation, no extra sandbox runtime)

Part Surface Op Result Expected Status
A MCP reads (list_issues, list_pull_requests, get_file_contents, list_commits) data returned ALLOWED
B MCP writes (add_issue_comment, star_repository, issue_write, create_branch, create_or_update_file, create_pull_request) all absent from tool catalog (unknown tool, -32602) — no write reached backend BLOCKED ⚠️
C CLI reads via github proxy CLI (list_issues, get_file_contents) data returned ALLOWED
D CLI REST writes (reaction/star/issue/comment) not attempted — gh auth status shows unauthenticated BLOCKED ⚠️
E CLI GraphQL mutations (addReaction/addStar/createIssue) not attempted — gh unauthenticated BLOCKED ⚠️

Overall: INCONCLUSIVE

⚠️ Part B: none of the 6 targeted write tools appear in the exposed 23-tool catalog for this gh-aw tools.github: workflow (read-only backend mode per gh-aw's own getGitHubReadOnly()), so their absence proves backend/toolset config, not mcpg's own gateway-level DIFC/guard blocking. No write was attempted or leaked.

⚠️ Parts D/E: gh CLI is unauthenticated in this sandbox (no token), so the token-scope write-boundary could not be exercised. No write was attempted or leaked.

No write succeeded on any surface. Independent evidence of mcpg's own write-blocking layer is covered by internal/guard/internal/difc unit tests, as noted in the test plan.

🔒 mcpg read-only stress (default AWF runtime) by Read-Only Stress: default runtime

@lpcox
lpcox merged commit ddd7f0b into main Sep 22, 2026
35 of 37 checks passed
@lpcox
lpcox deleted the test-improver/syncutil-registry-getorcreate-coverage-5f7eb7aecc26ae25 branch September 22, 2026 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants