testkit/mega: add mega test 'gen' and 'register' package#67770
Open
tiancaiamao wants to merge 7 commits intopingcap:masterfrom
Open
testkit/mega: add mega test 'gen' and 'register' package#67770tiancaiamao wants to merge 7 commits intopingcap:masterfrom
tiancaiamao wants to merge 7 commits intopingcap:masterfrom
Conversation
ti-chi-bot
bot
added
the
release-note-none
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
bot
added
the
size/XXL
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a concurrency-safe mega test Registry and Bazel target; relocates and adapts admin integration tests into a new Changes
Sequence Diagram(s)sequenceDiagram
participant T as testing.T
participant R as Registry
participant H as OnBeforeRun Hook
participant F as Test Function
T->>R: RunByPattern(pattern) / RunAll()
R->>R: collect matching pkg/name entries, sort deterministically
R->>H: snapshot hooks (read lock)
loop per matched package
R->>H: invoke hook(pkg) [once per package]
loop per test in package
R->>F: call test(t)
end
end
R->>T: return
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @tiancaiamao. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #67770 +/- ##
================================================
- Coverage 77.6020% 77.4575% -0.1445%
================================================
Files 1981 1968 -13
Lines 548804 549372 +568
================================================
- Hits 425883 425530 -353
- Misses 122111 123839 +1728
+ Partials 810 3 -807
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/testkit/mega/register/registry.go (3)
50-59: Consider detecting duplicate registrations.
Registersilently overwrites if the samepkg/nameis registered twice. This could mask accidental duplicate registrations. Consider logging a warning or panicking on duplicate to catch configuration errors early.🔧 Optional: detect duplicate registrations
func (r *Registry) Register(pkg, name string, fn func(*testing.T)) { r.mu.Lock() defer r.mu.Unlock() if r.tests[pkg] == nil { r.tests[pkg] = make(map[string]func(*testing.T)) } + if _, exists := r.tests[pkg][name]; exists { + panic(fmt.Sprintf("duplicate test registration: %s/%s", pkg, name)) + } r.tests[pkg][name] = fn }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/testkit/mega/register/registry.go` around lines 50 - 59, Registry.Register currently overwrites an existing r.tests[pkg][name] silently; before assigning, check if r.tests[pkg][name] already exists and emit a clear failure (either log a warning or panic) including the pkg and name so duplicate registrations are obvious. Modify the Register method (guarded by r.mu) to detect existing entry using r.tests[pkg][name] != nil and then call panic(fmt.Sprintf(...)) or use your project logger with the pkg/name details before returning/overwriting.
104-144: Snapshot function pointers directly to avoid redundant lock acquisition and TOCTOU.The current code snapshots only
pkg/namepairs, then callsr.Get()after releasing the lock. This pattern:
- Acquires an extra RLock per test execution
- Has a theoretical TOCTOU gap if the registry were modified between snapshot and execution
Store the function pointers directly during the initial snapshot to eliminate both concerns.
♻️ Proposed fix to snapshot function pointers directly
r.mu.RLock() var matched []struct { pkg string name string + fn func(*testing.T) } for pkg, tests := range r.tests { - for name := range tests { + for name, fn := range tests { fullName := pkg + "/" + name if re.MatchString(fullName) { matched = append(matched, struct { pkg string name string - }{pkg, name}) + fn func(*testing.T) + }{pkg, name, fn}) } } } hooks := make([]func(string), len(r.onBefore)) copy(hooks, r.onBefore) r.mu.RUnlock() if len(matched) == 0 { t.Fatalf("no tests matched pattern %q", pattern) } // Track which packages have had their hooks called pkgHookCalled := make(map[string]bool) for _, m := range matched { if !pkgHookCalled[m.pkg] { for _, hook := range hooks { hook(m.pkg) } pkgHookCalled[m.pkg] = true } // Call fn directly without t.Run wrapper so that runtime.Caller(1) // in testdata.LoadTestCases returns the correct function name. - testFn, ok := r.Get(m.pkg, m.name) - if !ok { - t.Fatalf("test %s/%s not found", m.pkg, m.name) - } - testFn(t) + m.fn(t) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/testkit/mega/register/registry.go` around lines 104 - 144, Snapshot the actual test function pointers while holding the registry lock instead of only storing pkg/name pairs and calling r.Get later; when iterating r.tests under r.mu.RLock() capture testFn := tests[name] (or the equivalent map lookup) into the matched slice (add a field fn func(*testing.T)), detect/match-miss and call t.Fatalf while still under the lock, then release the lock and iterate matched to run hooks from r.onBefore once per package and invoke the captured fn(t) directly—this removes the extra r.Get calls and the TOCTOU window.
151-181: Apply the same refactor here: snapshot function pointers directly.Same concern as
RunByPattern— store function pointers in the snapshot to avoid the redundantr.Get()call.♻️ Proposed fix
r.mu.RLock() type testEntry struct { pkg string name string + fn func(*testing.T) } var entries []testEntry for pkg, tests := range r.tests { - for name := range tests { - entries = append(entries, testEntry{pkg, name}) + for name, fn := range tests { + entries = append(entries, testEntry{pkg, name, fn}) } } hooks := make([]func(string), len(r.onBefore)) copy(hooks, r.onBefore) r.mu.RUnlock() pkgHookCalled := make(map[string]bool) for _, e := range entries { if !pkgHookCalled[e.pkg] { for _, fn := range hooks { fn(e.pkg) } pkgHookCalled[e.pkg] = true } // Call fn directly without t.Run wrapper so that runtime.Caller(1) // in testdata.LoadTestCases returns the correct function name. - fn, ok := r.Get(e.pkg, e.name) - if !ok { - t.Fatalf("test %s/%s not found", e.pkg, e.name) - } - fn(t) + e.fn(t) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/testkit/mega/register/registry.go` around lines 151 - 181, Snapshot the test function pointers instead of re-querying r.Get: update the local testEntry struct to include a fn field (the test function type), populate entries by iterating r.tests and storing each function (while still copying r.onBefore under r.mu lock), then after unlocking call the package hooks and invoke the stored fn directly (remove the r.Get usage). Key symbols: r.tests, r.onBefore, testEntry, and r.Get — keep the existing pkgHookCalled logic and only replace the lookup with the saved fn to avoid the redundant r.Get() call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/testkit/mega/register/registry.go`:
- Around line 50-59: Registry.Register currently overwrites an existing
r.tests[pkg][name] silently; before assigning, check if r.tests[pkg][name]
already exists and emit a clear failure (either log a warning or panic)
including the pkg and name so duplicate registrations are obvious. Modify the
Register method (guarded by r.mu) to detect existing entry using
r.tests[pkg][name] != nil and then call panic(fmt.Sprintf(...)) or use your
project logger with the pkg/name details before returning/overwriting.
- Around line 104-144: Snapshot the actual test function pointers while holding
the registry lock instead of only storing pkg/name pairs and calling r.Get
later; when iterating r.tests under r.mu.RLock() capture testFn := tests[name]
(or the equivalent map lookup) into the matched slice (add a field fn
func(*testing.T)), detect/match-miss and call t.Fatalf while still under the
lock, then release the lock and iterate matched to run hooks from r.onBefore
once per package and invoke the captured fn(t) directly—this removes the extra
r.Get calls and the TOCTOU window.
- Around line 151-181: Snapshot the test function pointers instead of
re-querying r.Get: update the local testEntry struct to include a fn field (the
test function type), populate entries by iterating r.tests and storing each
function (while still copying r.onBefore under r.mu lock), then after unlocking
call the package hooks and invoke the stored fn directly (remove the r.Get
usage). Key symbols: r.tests, r.onBefore, testEntry, and r.Get — keep the
existing pkgHookCalled logic and only replace the lookup with the saved fn to
avoid the redundant r.Get() call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8ec5af52-71ef-4a25-a388-2ca3260f0a68
⛔ Files ignored due to path filters (3)
pkg/testkit/mega/gen/BUILD.bazelis excluded by!**/gen/**pkg/testkit/mega/gen/main.gois excluded by!**/gen/**pkg/testkit/mega/gen/mega-gen-pkg.gois excluded by!**/gen/**
📒 Files selected for processing (3)
pkg/importsdk/BUILD.bazelpkg/testkit/mega/register/BUILD.bazelpkg/testkit/mega/register/registry.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/util/admin/test/admin_integration.go (1)
25-25: Add a doc comment for this exported runner.
RunAdminCheckTableCorruptedis exported on Line 25; please add a short comment describing that it is the mega-test runner entrypoint.💡 Proposed update
-func RunAdminCheckTableCorrupted(t *testing.T) { +// RunAdminCheckTableCorrupted executes the admin corruption check test case for mega test registration. +func RunAdminCheckTableCorrupted(t *testing.T) {As per coding guidelines: "Keep exported-symbol doc comments, and prefer semantic constraints over name restatement."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/admin/test/admin_integration.go` at line 25, Add a one-line doc comment above the exported function RunAdminCheckTableCorrupted describing its purpose as the mega-test runner entrypoint for the admin "check table corrupted" integration tests; ensure the comment is semantic (e.g., states it orchestrates subtests or setups for corruption checks) and follows Go doc comment style starting with the function name "RunAdminCheckTableCorrupted".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/util/admin/test/register_gen.go`:
- Around line 1-11: This generated file is missing the standard TiDB Apache-2.0
license header; add the canonical TiDB license header as a file-level comment at
the top of pkg/util/admin/test/register_gen.go and update the mega-gen
generation template(s) used to produce register_gen.go so all regenerated files
include that header automatically; ensure the header appears before the package
declaration (above "package test") and keep existing contents (the import and
init function that calls register.Register("util_admin",
"AdminCheckTableCorrupted", RunAdminCheckTableCorrupted)) unchanged.
---
Nitpick comments:
In `@pkg/util/admin/test/admin_integration.go`:
- Line 25: Add a one-line doc comment above the exported function
RunAdminCheckTableCorrupted describing its purpose as the mega-test runner
entrypoint for the admin "check table corrupted" integration tests; ensure the
comment is semantic (e.g., states it orchestrates subtests or setups for
corruption checks) and follows Go doc comment style starting with the function
name "RunAdminCheckTableCorrupted".
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3d97e509-ebb7-43ce-8c81-687558999253
⛔ Files ignored due to path filters (1)
pkg/testkit/mega/gen/main.gois excluded by!**/gen/**
📒 Files selected for processing (7)
pkg/util/admin/BUILD.bazelpkg/util/admin/main_test.gopkg/util/admin/test/BUILD.bazelpkg/util/admin/test/admin_integration.gopkg/util/admin/test/admin_integration_test.gopkg/util/admin/test/mega_gen.gopkg/util/admin/test/register_gen.go
💤 Files with no reviewable changes (1)
- pkg/util/admin/main_test.go
✅ Files skipped from review due to trivial changes (3)
- pkg/util/admin/test/mega_gen.go
- pkg/util/admin/test/admin_integration_test.go
- pkg/util/admin/test/BUILD.bazel
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
- Replace sb.WriteString(fmt.Sprintf(...)) with fmt.Fprintf(&sb, ...) (QF1012) - Preallocate allFuncs slices (prealloc)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/testkit/mega/register/registry.go`:
- Around line 17-21: The registry currently iterates maps in functions that list
or execute tests (e.g., the registry listing function, RunByPattern, RunAll and
any helper that builds the slice from a map); make iteration deterministic by
collecting map entries into a slice, sorting that slice by pkg then name
(lexicographically), and then returning or iterating that sorted slice for both
listing and execution. Update any places that build test lists (the code paths
covering the ranges around RunByPattern / RunAll and the registry list builder)
to use the same sort-by-pkg-name step before returning or calling t.Run so the
exposed order is stable across runs.
In `@pkg/util/admin/test/mega_gen.go`:
- Around line 1-3: Add the standard TiDB Apache-2.0 license header to the top of
the new source file `mega_gen.go` (above the `package test` and `//go:generate`
line); copy the canonical TiDB header (copyright line + Apache-2.0 notice) from
a nearby file and update the year if needed so the file begins with the full
TiDB license block before the package declaration and the go:generate directive.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 82468549-afaf-4b7c-b7db-d1b45bd0bc0d
⛔ Files ignored due to path filters (1)
pkg/testkit/mega/gen/main.gois excluded by!**/gen/**
📒 Files selected for processing (6)
go.modpkg/testkit/mega/register/registry.gopkg/util/admin/test/admin_integration.gopkg/util/admin/test/admin_integration_test.gopkg/util/admin/test/mega_gen.gopkg/util/admin/test/register_gen.go
✅ Files skipped from review due to trivial changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/util/admin/test/admin_integration.go
- admin_integration.go: use increment operator (revive) - admin_integration.go: explicitly ignore Set error (errcheck) - mega_gen.go: add missing Apache-2.0 license header
…c iteration - Capture function pointers under RLock to avoid redundant r.Get() calls and eliminate the TOCTOU window (CodeRabbit suggestion) - Sort test entries by pkg/name for deterministic iteration order - Extract snapshotAll helper shared by ListAll and RunAll
Member
|
/ok-to-test |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/testkit/mega/register/registry.go`:
- Around line 52-60: The Register method should guard against nil test functions
to fail fast: inside Registry.Register (which currently locks r.mu and writes
into r.tests), validate that fn != nil and if it is nil return a clear error
(e.g., panic or log.Fatalf) stating the package and test name; this prevents a
later panic in RunByPattern or RunAll when calling fn(t) and surfaces the
problem at registration time.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 348c5344-c0d5-4efb-bb68-e21434ab4218
📒 Files selected for processing (1)
pkg/testkit/mega/register/registry.go
Comment on lines
+52
to
+60
| func (r *Registry) Register(pkg, name string, fn func(*testing.T)) { | ||
| r.mu.Lock() | ||
| defer r.mu.Unlock() | ||
|
|
||
| if r.tests[pkg] == nil { | ||
| r.tests[pkg] = make(map[string]func(*testing.T)) | ||
| } | ||
| r.tests[pkg][name] = fn | ||
| } |
There was a problem hiding this comment.
Consider validating that fn is not nil.
If a nil function is registered (e.g., due to a code generation bug), it will cause a panic later during RunByPattern or RunAll when fn(t) is invoked. A defensive check here would surface the error at registration time with a clear message.
🛡️ Proposed defensive check
func (r *Registry) Register(pkg, name string, fn func(*testing.T)) {
+ if fn == nil {
+ panic("register: cannot register nil test function for " + pkg + "/" + name)
+ }
r.mu.Lock()
defer r.mu.Unlock()📝 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 (r *Registry) Register(pkg, name string, fn func(*testing.T)) { | |
| r.mu.Lock() | |
| defer r.mu.Unlock() | |
| if r.tests[pkg] == nil { | |
| r.tests[pkg] = make(map[string]func(*testing.T)) | |
| } | |
| r.tests[pkg][name] = fn | |
| } | |
| func (r *Registry) Register(pkg, name string, fn func(*testing.T)) { | |
| if fn == nil { | |
| panic("register: cannot register nil test function for " + pkg + "/" + name) | |
| } | |
| r.mu.Lock() | |
| defer r.mu.Unlock() | |
| if r.tests[pkg] == nil { | |
| r.tests[pkg] = make(map[string]func(*testing.T)) | |
| } | |
| r.tests[pkg][name] = fn | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/testkit/mega/register/registry.go` around lines 52 - 60, The Register
method should guard against nil test functions to fail fast: inside
Registry.Register (which currently locks r.mu and writes into r.tests), validate
that fn != nil and if it is nil return a clear error (e.g., panic or log.Fatalf)
stating the package and test name; this prevents a later panic in RunByPattern
or RunAll when calling fn(t) and surfaces the problem at registration time.
Contributor
Author
|
/retest |
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
2 participants
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.
What problem does this PR solve?
Issue Number: ref #67753
Problem Summary:
What changed and how does it work?
I've test in this branch #67679, that a huge monolithic test binary can save a lot link time.
bazel can accelerate build time by reuse .a cache, but it can not accelerate link time.
And compile .go to .a can run in parallel, but link phase remain on single core.
Even the simplest case take 20s+, even with build cache.
So the idea is that we just build all the tests in one binary to cut the link phase time.
To achieve that, we need some facilities.
We need to rewrite all the test from
TestXXXtoRunXXX, and move them fromxxx_test.gototest/xxx.goThen, use 'go generate' to generate
_test.goandTestXXXIn this way we have the same test as before
The next thing is, register the
RunXXXtests in init(), once they are registered, we can call them whereever we want.In this way we can collect all the tests into a single test binary.
util/admin/test is an example as how mega gen and register would be used.
Check List
Tests
Those code are meant to be used by 'go generate' and embed in 'RunXXX' later.
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Chores
Tests