test(auth): cover hyphenated whitelist owner matching#188
test(auth): cover hyphenated whitelist owner matching#188
Conversation
- Add regression coverage for jow-/ucode whitelist entries - Verify whitelist parsing and matching preserve hyphenated owner names
Walkthrough新增单元测试文件 Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new test file auth/whitelist_test.go to verify that hyphenated repository owners are correctly handled in the whitelist. The tests cover both the splitting logic and the initialization process. A review comment suggests moving the t.Cleanup registration to the beginning of the test to ensure global variables are reset even if an assertion fails, preventing side effects on other tests.
| whitelistInstance = nil | ||
| whitelistInitErr = nil | ||
|
|
||
| cfg := &config.Config{} | ||
| cfg.Whitelist.WhitelistFile = whitelistPath | ||
|
|
||
| if err := InitWhitelist(cfg); err != nil { | ||
| t.Fatalf("InitWhitelist() error = %v", err) | ||
| } | ||
|
|
||
| if !CheckWhitelist("jow-", "ucode") { | ||
| t.Fatal("CheckWhitelist() = false, want true for jow-/ucode") | ||
| } | ||
|
|
||
| if CheckWhitelist("jow", "ucode") { | ||
| t.Fatal("CheckWhitelist() = true, want false for non-hyphenated owner") | ||
| } | ||
|
|
||
| if CheckWhitelist("jow-", "other") { | ||
| t.Fatal("CheckWhitelist() = true, want false for unmatched repo") | ||
| } | ||
|
|
||
| t.Cleanup(func() { | ||
| whitelistInstance = nil | ||
| whitelistInitErr = nil | ||
| }) |
There was a problem hiding this comment.
The t.Cleanup function should be registered immediately after the global variables are modified. In its current position at the end of the test, it will not be executed if InitWhitelist fails or if any assertion fails before the registration point. Moving it up ensures that the global state is properly reset even on test failure, preventing side effects on other tests in the same package.
whitelistInstance = nil
whitelistInitErr = nil
t.Cleanup(func() {
whitelistInstance = nil
whitelistInitErr = nil
})
cfg := &config.Config{}
cfg.Whitelist.WhitelistFile = whitelistPath
if err := InitWhitelist(cfg); err != nil {
t.Fatalf("InitWhitelist() error = %v", err)
}
if !CheckWhitelist("jow-", "ucode") {
t.Fatal("CheckWhitelist() = false, want true for jow-/ucode")
}
if CheckWhitelist("jow", "ucode") {
t.Fatal("CheckWhitelist() = true, want false for non-hyphenated owner")
}
if CheckWhitelist("jow-", "other") {
t.Fatal("CheckWhitelist() = true, want false for unmatched repo")
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@auth/whitelist_test.go`:
- Around line 31-56: Save the current globals whitelistInstance and
whitelistInitErr into local variables and immediately register a t.Cleanup that
restores those saved values before performing the test actions (i.e. call
t.Cleanup right after declaring cfg and before calling
InitWhitelist/CheckWhitelist); this ensures that if InitWhitelist or any
t.Fatalf runs the cleanup is still registered and the globals are restored.
Refer to symbols: whitelistInstance, whitelistInitErr, InitWhitelist,
CheckWhitelist, and t.Cleanup when making the change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: acc1db43-0663-44b7-b689-c96ae66bc75d
📒 Files selected for processing (1)
auth/whitelist_test.go
| whitelistInstance = nil | ||
| whitelistInitErr = nil | ||
|
|
||
| cfg := &config.Config{} | ||
| cfg.Whitelist.WhitelistFile = whitelistPath | ||
|
|
||
| if err := InitWhitelist(cfg); err != nil { | ||
| t.Fatalf("InitWhitelist() error = %v", err) | ||
| } | ||
|
|
||
| if !CheckWhitelist("jow-", "ucode") { | ||
| t.Fatal("CheckWhitelist() = false, want true for jow-/ucode") | ||
| } | ||
|
|
||
| if CheckWhitelist("jow", "ucode") { | ||
| t.Fatal("CheckWhitelist() = true, want false for non-hyphenated owner") | ||
| } | ||
|
|
||
| if CheckWhitelist("jow-", "other") { | ||
| t.Fatal("CheckWhitelist() = true, want false for unmatched repo") | ||
| } | ||
|
|
||
| t.Cleanup(func() { | ||
| whitelistInstance = nil | ||
| whitelistInitErr = nil | ||
| }) |
There was a problem hiding this comment.
将 t.Cleanup 前置并恢复旧状态，避免失败路径污染全局变量。
当前在 Line 53 才注册清理；如果 Line 37-50 之间触发 t.Fatalf，清理函数不会注册，whitelistInstance/whitelistInitErr 可能泄漏到后续测试。建议在重置前保存旧值并立即注册 cleanup。
🛠️ 建议修改
- whitelistInstance = nil
- whitelistInitErr = nil
+ prevWhitelistInstance := whitelistInstance
+ prevWhitelistInitErr := whitelistInitErr
+ t.Cleanup(func() {
+ whitelistInstance = prevWhitelistInstance
+ whitelistInitErr = prevWhitelistInitErr
+ })
+ whitelistInstance = nil
+ whitelistInitErr = nil
@@
- t.Cleanup(func() {
- whitelistInstance = nil
- whitelistInitErr = nil
- })Based on learnings: The blacklist and whitelist functionality in auth/ package uses maps for O(1) lookups, separates user-level and repo-level controls, and ensures thread safety using sync.Once.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@auth/whitelist_test.go` around lines 31 - 56, Save the current globals
whitelistInstance and whitelistInitErr into local variables and immediately
register a t.Cleanup that restores those saved values before performing the test
actions (i.e. call t.Cleanup right after declaring cfg and before calling
InitWhitelist/CheckWhitelist); this ensures that if InitWhitelist or any
t.Fatalf runs the cleanup is still registered and the globals are restored.
Refer to symbols: whitelistInstance, whitelistInitErr, InitWhitelist,
CheckWhitelist, and t.Cleanup when making the change.
2 participants
Summary
jow-/ucodeCheckWhitelistmatching preserve the trailing hyphen in the owner namemainTesting
go test ./auth -run 'Test(SplitUserRepoWhitelist_PreservesHyphenatedOwner|InitWhitelist_AllowsHyphenatedOwnerRepo)$'Summary by CodeRabbit
发布说明