optimize #27
optimize #27
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a release workflow; expands configuration (retry, concurrency, cache, notifications, reports, environments); implements an in-memory cache manager and usage patterns; introduces a pooled Lua VM; adds validation helpers/tests; reporting, notification, rate-limiter, and retry utilities; an environment manager and CLI env flag; plus docs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as main.go (CLI)
participant Cmd as command.RunTestCases
participant Res as resource.InitCacheManager
participant VM as luavm.GlobalLuaVMPool
participant HTTP as HTTP Exec
participant GRPC as gRPC Exec
participant Rep as util.ReportGenerator
participant Ntf as util.NotificationService
User->>CLI: run --env=staging
CLI->>Cmd: RunTestCases(env)
Cmd->>Res: InitCacheManager()
Cmd->>VM: Use pooled Lua for rendering
alt HTTP GET
Cmd->>HTTP: executeHttpRequestWithCache()
HTTP->>HTTP: Cache get -> miss -> network -> cache set (2xx)
end
alt gRPC Descriptor
Cmd->>GRPC: fetch descriptor (new cache -> old cache -> create)
end
Cmd-->>Cmd: Execute testcases
Cmd->>Cmd: outputCacheStats()
Cmd->>Rep: GenerateReports(formats)
Cmd->>Ntf: SendTestResult()
Ntf-->>Cmd: ack
Cmd-->>CLI: exit
sequenceDiagram
autonumber
participant R as HTTP Request (GET)
participant C as HttpResponseCache
participant Net as Network
R->>C: Get(key)
alt cache hit
C-->>R: Cached response
else cache miss
R->>Net: Send request
Net-->>R: Response (status)
alt status 2xx
R->>C: Set(key, response)
end
R-->>R: Return response
end
sequenceDiagram
autonumber
participant G as GRPC Call
participant NC as GrpcDescriptorCache (new)
participant OC as DescSourceCache (old)
participant F as Descriptor Fetch
G->>NC: Get(address)
alt new cache hit
NC-->>G: DescriptorSource
else miss
G->>OC: Get(address)
alt old cache hit
OC-->>G: DescriptorSource
G->>NC: Set(address, ds)
else miss
G->>F: Create DescriptorSource
F-->>G: ds
G->>NC: Set(address, ds)
G->>OC: Set(address, ds)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (9)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/rule/grpc_lua_rule.go (1)
39-47: Setter uses wrong argument index; unify to 2 (Lua “self, value”)Lua method calls pass receiver + value → top should be 2 for setters. Body currently reads index 3 (bug); Code checks top==3 (inconsistent).
Apply this diff:
func getSetGrpcRespCode(L *lua.LState) int { p := checkGrpcResp(L) - if L.GetTop() == 3 { + if L.GetTop() == 2 { p.Code = L.CheckString(2) return 0 } L.Push(lua.LString(p.Code)) return 1 } func getSetGrpcRespBody(L *lua.LState) int { p := checkGrpcResp(L) - if L.GetTop() == 3 { - p.Body = L.CheckString(3) + if L.GetTop() == 2 { + p.Body = L.CheckString(2) return 0 } L.Push(lua.LString(p.Body)) return 1 }Also applies to: 50-57
internal/rule/http_lua_rule.go (1)
25-34: Fix setter argument indices (use 2, not 3) for Lua methodsLua method calls push “self” then the value; setters should read index 2 and check top==2.
Apply this diff:
func getSetHttpRespCode(L *lua.LState) int { p := checkHttpResp(L) - if L.GetTop() == 3 { + if L.GetTop() == 2 { p.Code = L.CheckString(2) return 0 } L.Push(lua.LString(p.Code)) return 1 } func getSetHttpRespBody(L *lua.LState) int { p := checkHttpResp(L) - if L.GetTop() == 3 { - p.Body = L.CheckString(3) + if L.GetTop() == 2 { + p.Body = L.CheckString(2) return 0 } L.Push(lua.LString(p.Body)) return 1 }Also applies to: 37-45
🧹 Nitpick comments (9)
internal/luavm/init.go (1)
145-165: Consider adding recursion depth limit for table copying.The
copyLuaValuefunction recursively copies Lua tables without a depth limit. Deeply nested tables could cause a stack overflow.Consider adding a depth limit to prevent stack overflow on deeply nested tables:
// copyLuaValue 复制Lua值,避免引用问题 func copyLuaValue(L *lua.LState, value lua.LValue) lua.LValue { return copyLuaValueWithDepth(L, value, 0, 100) } // copyLuaValueWithDepth 复制Lua值,带深度限制 func copyLuaValueWithDepth(L *lua.LState, value lua.LValue, depth, maxDepth int) lua.LValue { if depth > maxDepth { // 超过最大深度,返回字符串表示 return lua.LString(value.String()) } switch v := value.(type) { case lua.LString: return v // 字符串是不可变的,可以直接返回 case lua.LNumber: return v // 数字是不可变的,可以直接返回 case lua.LBool: return v // 布尔值是不可变的,可以直接返回 case *lua.LTable: // 对于表,创建一个新的表并复制内容 newTable := L.NewTable() v.ForEach(func(key, val lua.LValue) { newTable.RawSet(key, copyLuaValueWithDepth(L, val, depth+1, maxDepth)) }) return newTable default: // 对于其他类型,返回字符串表示 return lua.LString(value.String()) } }internal/util/notification.go (1)
178-186: Include response body in webhook error for easier debuggingReturn the response body when status != 200. This often contains Slack error details.
Apply this diff:
- resp, err := ns.client.Do(req) - if err != nil { - return fmt.Errorf("failed to send webhook: %w", err) - } - defer resp.Body.Close() - - if resp.StatusCode != http.StatusOK { - return fmt.Errorf("webhook returned status code: %d", resp.StatusCode) - } + resp, err := ns.client.Do(req) + if err != nil { + return fmt.Errorf("failed to send webhook: %w", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + b, _ := io.ReadAll(resp.Body) + return fmt.Errorf("webhook returned status code: %d, body: %s", resp.StatusCode, string(b)) + }Add missing import:
// at the top import block import ( "io" )internal/command/command.go (1)
285-303: Avoid panics from strict type assertions in cache statsGetStats may return mixed numeric types; the current .(int64)/(int) assertions can panic. Log values without asserting types or normalize with safe conversion.
Apply this diff:
func outputCacheStats() { if resource.CacheManager == nil { return } stats := resource.CacheManager.GetStats() slog.Info("=== Cache Statistics ===") for name, stat := range stats { - hits := stat["hits"].(int64) - misses := stat["misses"].(int64) - hitRate := stat["hit_rate"].(float64) - size := stat["size"].(int) - - slog.Info("Cache [%s]: Hits=%d, Misses=%d, HitRate=%.2f%%, Size=%d", - name, hits, misses, hitRate, size) + slog.Info("Cache [%s]: Hits=%v, Misses=%v, HitRate=%v%%, Size=%v", + name, stat["hits"], stat["misses"], stat["hit_rate"], stat["size"]) } }Would you confirm the exact types returned by CacheManager.GetStats() so we can tighten formatting without risking panics?
internal/util/cache.go (3)
85-117: Consider atomic counters for hit/miss statistics.The current implementation acquires a write lock multiple times to update
hitsandmissescounters. For high-throughput scenarios, this could become a contention point. Consider usingsync/atomicoperations for these counters to avoid lock contention:import "sync/atomic" type Cache struct { // ... existing fields ... hits atomic.Int64 misses atomic.Int64 } // In Get method: if !exists { atomic.AddInt64(&c.misses, 1) return nil, false }This would eliminate the need for write locks when updating statistics.
179-197: Consider more efficient eviction strategy.The current LRU eviction scans all cache entries (O(n)) while holding the write lock. For large caches, this could cause performance degradation. Consider using:
- A heap-based priority queue for O(log n) eviction
- A doubly-linked list with hash map for O(1) LRU eviction (e.g., using
container/list)However, given the default max size of 1000 entries, this may be acceptable for current use cases.
199-224: Cleanup goroutine could block cache operations.The cleanup routine holds the write lock for the entire duration of scanning and deleting expired items. For large caches or slow expiration checks, this could block concurrent Get/Set operations. Consider:
- Batch the deletions and release/reacquire the lock between batches
- Use a two-phase approach: collect expired keys with read lock, then delete with write lock
However, for the default cache size (1000 entries), this may not be a practical concern.
internal/util/cache_usage_example.go (3)
121-134: Consider including more cache-affecting headers.The current implementation only includes
authorizationandcontent-typeheaders in the cache key. Consider also including:
Accept(response format)Accept-Language(localization)Accept-Encoding(compression)These headers can affect the response content and should ideally be part of the cache key. However, for a basic implementation, the current approach may be sufficient.
163-180: Add safety checks for type assertions.The function performs multiple type assertions without checking if they succeed:
Hits: stat["hits"].(int64), Misses: stat["misses"].(int64), HitRate: stat["hit_rate"].(float64), Size: stat["size"].(int),While these should be safe since
GetStats()in the same package controls the types, consider adding checks for robustness or at minimum document that this function expects a specific format fromGetStats().
182-209: Warmup implementation is a simplified placeholder.The comment indicates this is a "简化示例" (simplified example), and the code just stores the template string directly rather than pre-rendering templates. This is fine for example code, but note that a production warmup would:
- Actually render templates with common variables
- Store the rendered results (not just the template string)
- Potentially load frequently-used gRPC descriptors and HTTP responses
Consider adding a TODO comment indicating this is a placeholder for actual warmup logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.github/workflows/release.yml(1 hunks)config_files/autotest_enhanced.yml(1 hunks)doc/cache_guide.md(1 hunks)doc/usage.md(1 hunks)internal/command/command.go(3 hunks)internal/command/grpc_call.go(5 hunks)internal/command/http_call.go(4 hunks)internal/command/validation_helper.go(1 hunks)internal/command/validation_helper_test.go(1 hunks)internal/config/config.go(2 hunks)internal/luavm/init.go(1 hunks)internal/resource/resource.go(3 hunks)internal/rule/grpc_lua_rule.go(1 hunks)internal/rule/http_lua_rule.go(2 hunks)internal/util/cache.go(1 hunks)internal/util/cache_usage_example.go(1 hunks)internal/util/environment.go(1 hunks)internal/util/notification.go(1 hunks)internal/util/ratelimit.go(1 hunks)internal/util/report.go(1 hunks)internal/util/retry.go(1 hunks)main.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (15)
internal/command/validation_helper_test.go (2)
internal/command/validation_helper.go (4)
ValidateTestCaseIDs(12-24)ValidateDependencies(27-53)ValidateXPath(56-63)ValidateBodyFields(66-72)internal/command/command.go (3)
ErrorIDduplicate(19-19)ErrorDependencyNotExist(20-20)ErrorIDRestrict(21-21)
internal/util/notification.go (1)
internal/config/config.go (1)
AutoTestConfig(9-65)
internal/command/grpc_call.go (4)
internal/luavm/init.go (1)
ExecuteLuaWithGlobalsPool(94-136)internal/resource/resource.go (2)
CacheManager(37-37)DescSourceCache(36-36)internal/util/cache.go (1)
CacheManager(227-233)internal/model/model.go (1)
DescSourceCache(73-76)
internal/util/environment.go (1)
internal/config/config.go (1)
AutoTestConfig(9-65)
internal/command/http_call.go (3)
internal/luavm/init.go (1)
ExecuteLuaWithGlobalsPool(94-136)internal/resource/resource.go (1)
CacheManager(37-37)internal/util/cache.go (1)
CacheManager(227-233)
internal/rule/grpc_lua_rule.go (2)
internal/luavm/init.go (1)
ExecuteLuaWithGlobalsPool(94-136)internal/rule/grpc_lua_rule_test.go (1)
TestGrpcLuaRule(9-21)
internal/resource/resource.go (1)
internal/util/cache.go (2)
CacheManager(227-233)NewCacheManager(236-243)
internal/config/config.go (1)
internal/util/cache.go (1)
Cache(23-33)
internal/util/retry.go (1)
internal/config/config.go (1)
AutoTestConfig(9-65)
internal/command/validation_helper.go (2)
internal/model/model.go (1)
IdItem(63-65)internal/command/command.go (3)
ErrorIDduplicate(19-19)ErrorDependencyNotExist(20-20)ErrorIDRestrict(21-21)
internal/util/report.go (3)
internal/config/config.go (1)
AutoTestConfig(9-65)internal/command/grpc_automate.go (1)
GenReportFileGrpc(152-209)internal/command/http_automate.go (1)
GenReportFileHttp(60-118)
internal/util/cache_usage_example.go (2)
internal/util/cache.go (2)
Cache(23-33)CacheManager(227-233)internal/resource/resource.go (1)
CacheManager(37-37)
internal/util/cache.go (2)
internal/config/config.go (1)
AutoTestConfig(9-65)internal/resource/resource.go (1)
CacheManager(37-37)
internal/rule/http_lua_rule.go (2)
internal/luavm/init.go (1)
ExecuteLuaWithGlobalsPool(94-136)internal/rule/lua_test.go (1)
TestLua(10-30)
internal/command/command.go (2)
internal/resource/resource.go (3)
GlobalConfig(26-26)InitCacheManager(50-53)CacheManager(37-37)internal/util/cache.go (1)
CacheManager(227-233)
🪛 actionlint (1.7.8)
.github/workflows/release.yml
17-17: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
44-44: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 GitHub Actions: golang-ci
internal/util/environment.go
[error] 167-167: Error return value of file.WriteString is not checked (errcheck)
[error] 168-168: Error return value of file.WriteString is not checked (errcheck)
[error] 169-169: S1025: the argument is already a string, there's no need to use fmt.Sprintf (staticcheck)
[error] 177-177: QF1012: Use fmt.Fprintf(...) instead of WriteString(fmt.Sprintf(...)) (staticcheck)
internal/luavm/init.go
[error] 65-65: Error return value of L.DoString is not checked (errcheck)
[error] 95-95: shadow: declaration of "L" shadows declaration at line 85 (govet)
[error] 120-120: ST1023: should omit type lua.LValue from declaration; it will be inferred from the right-hand side (staticcheck)
internal/util/report.go
[error] 266-266: Error return value of file.WriteString is not checked (errcheck)
🪛 LanguageTool
doc/cache_guide.md
[uncategorized] ~192-~192: 动词的修饰一般为‘形容词（副词）+地+动词’。您的意思是否是：重要"地"请求
Context: ... 缓存键设计 缓存系统会自动生成缓存键，包含： - 请求方法和URL - 重要的请求头（如Authorization） - 模板内容和变量 - Lua脚本内容 ...
(wb4)
🔇 Additional comments (44)
main.go (2)
6-8: LGTM!The import additions are correctly positioned and follow Go conventions.
45-45: LGTM!The new environment flag follows the existing CLI pattern and provides clear usage guidance.
doc/usage.md (1)
1-223: LGTM!The usage guide is comprehensive and well-structured, covering quick start, configuration, test case writing, advanced features, best practices, and troubleshooting. The documentation aligns well with the new features introduced in this PR.
doc/cache_guide.md (1)
1-253: LGTM!The cache guide provides comprehensive documentation of the AutoTest caching system, covering all cache types (gRPC descriptor, HTTP response, template, and Lua script caches), configuration options, performance optimization tips, statistics monitoring, best practices, and troubleshooting guidance. The examples and scenarios are clear and well-explained.
internal/util/retry.go (2)
27-74: LGTM! Consider documenting retry safety.The retry logic is well-implemented with proper error handling, configurable attempts and delays, and status code checking. The implementation correctly returns early on success and waits between retries only when needed.
Consider adding a comment documenting that this retry mechanism is safe for idempotent requests only. For non-idempotent operations (POST, PUT, DELETE without idempotency keys), retrying on network errors could lead to duplicate operations. You might want to verify that callers are aware of this limitation:
// ExecuteWithRetry 执行HTTP请求并支持重试 +// Note: This retry mechanism is suitable for idempotent requests (GET, HEAD, OPTIONS, TRACE). +// For non-idempotent operations (POST, PUT, DELETE), ensure idempotency keys or other +// safeguards are in place to prevent duplicate operations on retry. func (r *RetryableHTTPClient) ExecuteWithRetry(req *resty.Request) (*resty.Response, error) {
77-95: LGTM!The
IsRetryableStatusCodehelper correctly implements the retry logic with sensible defaults (500, 502, 503, 504, 408, 429) and allows configuration via theretryCodesparameter.internal/util/ratelimit.go (4)
23-46: LGTM!The rate limiter implementation correctly combines a semaphore for concurrency control with a token bucket for rate limiting. The initial token fill and goroutine launch are properly structured.
48-62: LGTM!The token generator correctly uses a ticker to produce tokens at the configured rate and respects the stop signal for clean shutdown.
64-83: LGTM!The
Acquiremethod correctly implements two-phase acquisition (semaphore first, then token) and properly cleans up the semaphore on context cancellation during token acquisition.
85-108: LGTM!The
Release,Stop, andExecuteWithLimitmethods are correctly implemented. The use ofsync.OnceinStopensures idempotent shutdown, andExecuteWithLimitprovides a convenient wrapper with proper cleanup via defer.internal/config/config.go (2)
16-59: LGTM!The new configuration sections (Retry, Concurrency, Cache, Logger.Rotation, Report enhancements, and Notifications) are well-structured with appropriate types and YAML tags. These additions align with the enhanced features introduced in this PR.
64-64: LGTM!The
Environmentsmap provides a flexible way to manage environment-specific variables, supporting the new environment management features.internal/luavm/init.go (4)
10-23: LGTM!The
LuaVMPooltype correctly wraps async.Poolto provide pooled Lua VM instances. The global pool initialization properly creates new Lua states with the JSON library preloaded.
25-35: LGTM!The
GetLuaStateandPutLuaStatemethods correctly implement pool acquisition and return with proper cleanup before returning to the pool.
68-82: LGTM!The
isBuiltinGlobalfunction correctly identifies Lua's built-in globals to avoid accidentally removing them during cleanup. The list is comprehensive and includes the preloadedjsonlibrary.
138-143: LGTM!The
cleanupGlobalshelper correctly clears the specified global variables by setting them tolua.LNil.internal/rule/grpc_lua_rule.go (1)
89-89: Migration to pooled Lua VM is goodSwitching to ExecuteLuaWithGlobalsPool aligns with VM pooling and isolation.
internal/rule/http_lua_rule.go (1)
95-95: Migration to pooled Lua VM is goodUsing ExecuteLuaWithGlobalsPool is consistent with new Lua VM pooling.
internal/command/command.go (2)
52-59: Init flow LGTMInitializing Resty and the CacheManager up-front is correct and aligns with downstream cache usage.
65-68: Good addition: emit cache stats post-runSurfacing cache metrics improves observability of the new cache subsystem.
internal/command/http_call.go (3)
174-191: GET caching integration looks good; ensure helper is nil‑safeThe switch to cache GETs is fine. Just make sure executeHttpRequestWithCache gracefully bypasses caching if CacheManager/HttpResponseCache is nil to avoid panics.
I’ve proposed a nil‑guard in the helper below; please confirm all call paths initialize the CacheManager (RunTestCases does).
261-261: Lua VM pool usage LGTMPooling via ExecuteLuaWithGlobalsPool is correct and safe with nil globals.
278-304: No unsafe CacheManager dereferences foundThe scan confirms that
resource.CacheManageris guaranteed to be non-nil in all dereference locations. TheNewCacheManager()function at internal/util/cache.go:236 always returns a properly initialized*CacheManagerstruct—it has no failure paths or nil returns. SinceCacheManageris assigned once at startup (internal/resource/resource.go:51) and never reassigned, all dereferences at http_call.go:284, 300 and grpc_call.go:282, 293, 317 are safe.The defensive nil-check in
outputCacheStats()at command.go:287 is unnecessary but harmless.internal/command/grpc_call.go (1)
262-262: Lua VM pool usage LGTMSwitch to ExecuteLuaWithGlobalsPool is correct here as well.
internal/command/validation_helper_test.go (1)
1-186: Solid, focused tests for the new validation helpersCovers duplicates, dependencies, XPath validity, and body/luaBody exclusivity with clear table-driven cases. Good use of testify.
config_files/autotest_enhanced.yml (3)
2-6: LGTM!The basic global settings are well-defined with sensible defaults.
8-23: Verify interaction between concurrency and rate limiting.The configuration defines both
max_concurrent_requests: 10andrate_limit_per_second: 100. Ensure the implementation correctly handles the case where rate limiting could allow more concurrent requests than the max_concurrent_requests setting, or vice versa.
54-64: LGTM!The environment configuration structure is clean and supports multiple deployment environments effectively.
internal/command/validation_helper.go (4)
11-24: LGTM!The duplicate ID detection logic is correct and uses an efficient set-based approach with O(n) time complexity.
26-53: LGTM! Dependency ordering prevents cycles.The validation correctly enforces that dependencies must have IDs strictly smaller than the dependent test case. This prevents forward references and helps maintain a valid execution order (topological sort).
55-63: LGTM!XPath validation correctly delegates to the xpath library's compile function to detect syntax errors.
65-72: LGTM! Mutual exclusion validation is correct.The function correctly prevents both
bodyandluaBodyfrom being set simultaneously. Note that both fields being empty strings is allowed, which appears intentional for cases where the body is optional.internal/util/cache.go (7)
11-33: LGTM! Thread-safe cache design.The cache structure is well-designed with:
- RWMutex for concurrent access
- TTL-based expiration
- Statistics tracking (hits/misses)
- Size limiting (maxSize)
35-61: LGTM! Constructor with sensible defaults.The constructor correctly:
- Reads configuration
- Applies reasonable defaults (5 min TTL, 1000 entries)
- Starts cleanup goroutine only when enabled
63-83: LGTM! Thread-safe cache insertion.The Set method correctly:
- Returns early when cache is disabled
- Evicts oldest entry when at capacity
- Sets proper expiration time
- Uses write lock for thread safety
119-157: LGTM! Standard cache operations.Delete, Clear, and Size methods are correctly implemented with proper locking.
159-177: LGTM! Safe hit rate calculation.The Stats method correctly handles the edge case of zero total accesses to avoid division by zero.
226-243: LGTM! Clean cache manager pattern.The CacheManager provides a clear separation of concerns with dedicated caches for different use cases (gRPC descriptors, HTTP responses, templates, Lua scripts).
245-276: LGTM! Manager operations.GetStats and ClearAll provide clean aggregation and bulk operations across all managed caches.
internal/util/cache_usage_example.go (5)
16-36: LGTM! Clean cache-aside pattern.The function correctly implements a cache-aside pattern:
- Check cache first
- On miss, call the provided callback
- Store result in cache
- Return result
38-69: LGTM! Appropriate HTTP caching strategy.The function correctly:
- Only caches GET requests (idempotent)
- Includes important headers in cache key
- Only caches successful responses (2xx status codes)
- Avoids caching errors that might be transient
71-94: LGTM! Efficient Lua script caching.Caching compiled Lua scripts by content hash avoids recompilation overhead. MD5 is appropriate here since it's used for cache keying, not cryptographic security.
96-119: LGTM! Template caching implementation.The function correctly caches rendered template results, including both template content and variables in the cache key.
136-139: LGTM! Script hash generation.Simple and correct MD5 hash for cache key generation.
| - name: Set up Go | ||
| uses: actions/setup-go@v4 | ||
| with: | ||
| go-version: '1.24' |
There was a problem hiding this comment.
Invalid Go version specified.
Go 1.24 does not exist yet. As of the current date, the latest stable Go version is 1.23.x.
Apply this diff to use a valid Go version:
- name: Set up Go
uses: actions/setup-go@v4
with:
- go-version: '1.24'
+ go-version: '1.23'Additionally, the static analysis tool flagged that actions/setup-go@v4 is too old to run on GitHub Actions. Consider updating to a newer version:
- name: Set up Go
- uses: actions/setup-go@v4
+ uses: actions/setup-go@v5
with:
- go-version: '1.24'
+ go-version: '1.23'📝 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.
| - name: Set up Go | |
| uses: actions/setup-go@v4 | |
| with: | |
| go-version: '1.24' | |
| - name: Set up Go | |
| uses: actions/setup-go@v5 | |
| with: | |
| go-version: '1.23' |
🧰 Tools
🪛 actionlint (1.7.8)
17-17: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In .github/workflows/release.yml around lines 16 to 19, the workflow pins a
non-existent Go version (1.24) and references an outdated actions/setup-go
release; change go-version to a valid current stable (e.g., "1.23" or "1.23.x")
and update the action reference to a newer release (e.g., actions/setup-go@v5 or
the latest available) so the workflow uses an existing Go toolchain and a
supported setup-go action.
| - name: Create Release | ||
| uses: softprops/action-gh-release@v1 | ||
| with: | ||
| files: | | ||
| autotest-* | ||
| checksums.txt | ||
| coverage.html | ||
| generate_release_notes: true | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
Update GitHub Action version.
Static analysis indicates that softprops/action-gh-release@v1 is too old to run on GitHub Actions.
Apply this diff to update to a newer version:
- name: Create Release
- uses: softprops/action-gh-release@v1
+ uses: softprops/action-gh-release@v2
with:
files: |
autotest-*
checksums.txt
coverage.html
generate_release_notes: true
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}📝 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.
| - name: Create Release | |
| uses: softprops/action-gh-release@v1 | |
| with: | |
| files: | | |
| autotest-* | |
| checksums.txt | |
| coverage.html | |
| generate_release_notes: true | |
| env: | |
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | |
| - name: Create Release | |
| uses: softprops/action-gh-release@v2 | |
| with: | |
| files: | | |
| autotest-* | |
| checksums.txt | |
| coverage.html | |
| generate_release_notes: true | |
| env: | |
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
🧰 Tools
🪛 actionlint (1.7.8)
44-44: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
In .github/workflows/release.yml around lines 43 to 52, the workflow uses the
outdated softprops/action-gh-release@v1; update the uses entry to a newer,
maintained tag (for example softprops/action-gh-release@v1.3.0 or the latest v2
release) by replacing the uses line accordingly, then commit and run the
workflow to verify the action executes without the old compatibility issues.
| logger: | ||
| level: "debug" | ||
| file_path: "/var/log/test/autotest.log" | ||
| # 新增：日志轮转 | ||
| rotation: | ||
| max_size: 100MB | ||
| max_age: 7d | ||
| max_backups: 10 | ||
|
|
||
| report: | ||
| dir_path: "/var/log/test/report/" | ||
| # 新增：报告格式 | ||
| formats: ["html", "json", "csv", "junit"] | ||
| # 新增：报告模板 | ||
| template_path: "./templates/report.html" |
There was a problem hiding this comment.
Use relative paths or document path customization requirements.
The configuration uses absolute paths (/var/log/test/autotest.log, /var/log/test/report/) which are not portable across different environments or operating systems. Consider using relative paths, environment variables, or clearly document in a comment that these paths must be customized per deployment.
Apply this diff to add clarifying comments:
logger:
level: "debug"
- file_path: "/var/log/test/autotest.log"
+ file_path: "/var/log/test/autotest.log" # TODO: Customize this path for your environment
# 新增：日志轮转
rotation:
max_size: 100MB
max_age: 7d
max_backups: 10
report:
- dir_path: "/var/log/test/report/"
+ dir_path: "/var/log/test/report/" # TODO: Customize this path for your environment📝 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.
| logger: | |
| level: "debug" | |
| file_path: "/var/log/test/autotest.log" | |
| # 新增：日志轮转 | |
| rotation: | |
| max_size: 100MB | |
| max_age: 7d | |
| max_backups: 10 | |
| report: | |
| dir_path: "/var/log/test/report/" | |
| # 新增：报告格式 | |
| formats: ["html", "json", "csv", "junit"] | |
| # 新增：报告模板 | |
| template_path: "./templates/report.html" | |
| logger: | |
| level: "debug" | |
| file_path: "/var/log/test/autotest.log" # TODO: Customize this path for your environment | |
| # 新增：日志轮转 | |
| rotation: | |
| max_size: 100MB | |
| max_age: 7d | |
| max_backups: 10 | |
| report: | |
| dir_path: "/var/log/test/report/" # TODO: Customize this path for your environment | |
| # 新增：报告格式 | |
| formats: ["html", "json", "csv", "junit"] | |
| # 新增：报告模板 | |
| template_path: "./templates/report.html" |
🤖 Prompt for AI Agents
In config_files/autotest_enhanced.yml around lines 25 to 39 the logger and
report paths are hard-coded as absolute paths which reduces portability; update
the file to use relative paths (e.g., ./logs/autotest.log and ./logs/report/),
or reference environment variables (e.g., a LOG_DIR/REPORT_DIR) and add a brief
comment above each path explaining that these should be adjusted per deployment
or set via env vars, including an example env var name and fallback default;
ensure the comments clearly state that absolute paths can be replaced and
document expected permissions for the target directories.
| # 新增：通知配置 | ||
| notifications: | ||
| enabled: true | ||
| webhook_url: "https://hooks.slack.com/services/..." | ||
| on_failure: true | ||
| on_success: false |
There was a problem hiding this comment.
Document webhook URL placeholder.
The webhook URL contains a placeholder (https://hooks.slack.com/services/...) that should be replaced with an actual webhook URL. Add a comment to clarify this is a placeholder that must be customized.
Apply this diff:
notifications:
enabled: true
- webhook_url: "https://hooks.slack.com/services/..."
+ webhook_url: "https://hooks.slack.com/services/..." # TODO: Replace with your actual webhook URL
on_failure: true
on_success: false📝 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.
| # 新增：通知配置 | |
| notifications: | |
| enabled: true | |
| webhook_url: "https://hooks.slack.com/services/..." | |
| on_failure: true | |
| on_success: false | |
| # 新增：通知配置 | |
| notifications: | |
| enabled: true | |
| webhook_url: "https://hooks.slack.com/services/..." # TODO: Replace with your actual webhook URL | |
| on_failure: true | |
| on_success: false |
🤖 Prompt for AI Agents
In config_files/autotest_enhanced.yml around lines 41 to 46 the
notifications.webhook_url is set to a placeholder
("https://hooks.slack.com/services/..."); update the file by adding a clear
inline comment that this value is a placeholder and must be replaced with a real
webhook URL for your Slack (or other) integration, and optionally include
guidance on where to obtain the webhook (e.g., Slack incoming webhook settings)
and a reminder not to commit secrets to version control.
| // 尝试从新缓存系统获取 | ||
| if cached, found := resource.CacheManager.GrpcDescriptorCache.Get(address); found { | ||
| if desc, ok := cached.(grpcurl.DescriptorSource); ok { | ||
| slog.Debug("Using cached gRPC descriptor for %s", address) | ||
| return desc, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Guard descriptor cache usage when CacheManager is nil
Wrap Get/Set calls to avoid panics if the cache manager isn’t initialized (e.g., alternate code paths/tests).
Apply this diff:
func getDescSourceWitchCache(ctx context.Context, address string) (grpcurl.DescriptorSource, error) {
- // 尝试从新缓存系统获取
- if cached, found := resource.CacheManager.GrpcDescriptorCache.Get(address); found {
- if desc, ok := cached.(grpcurl.DescriptorSource); ok {
- slog.Debug("Using cached gRPC descriptor for %s", address)
- return desc, nil
- }
- }
+ // 尝试从新缓存系统获取
+ if resource.CacheManager != nil && resource.CacheManager.GrpcDescriptorCache != nil {
+ if cached, found := resource.CacheManager.GrpcDescriptorCache.Get(address); found {
+ if desc, ok := cached.(grpcurl.DescriptorSource); ok {
+ slog.Debug("Using cached gRPC descriptor for %s", address)
+ return desc, nil
+ }
+ }
+ }
// 检查旧缓存系统
s, found := resource.DescSourceCache.Get(address)
if found {
- // 从旧缓存获取到了，也存储到新缓存
- resource.CacheManager.GrpcDescriptorCache.Set(address, s)
+ // 从旧缓存获取到了，也存储到新缓存
+ if resource.CacheManager != nil && resource.CacheManager.GrpcDescriptorCache != nil {
+ resource.CacheManager.GrpcDescriptorCache.Set(address, s)
+ }
return s, nil
}
@@
- // 同时存储到新旧缓存系统
+ // 同时存储到新旧缓存系统
resource.DescSourceCache.Set(address, s)
- resource.CacheManager.GrpcDescriptorCache.Set(address, s)
+ if resource.CacheManager != nil && resource.CacheManager.GrpcDescriptorCache != nil {
+ resource.CacheManager.GrpcDescriptorCache.Set(address, s)
+ }
return s, err
}Optional: consider renaming the function to getDescSourceWithCache (typo) in a separate PR to avoid noisy diffs.
Also applies to: 293-295, 315-318
🤖 Prompt for AI Agents
In internal/command/grpc_call.go around lines 281 to 287 (and similarly at
293-295 and 315-318), the code assumes resource.CacheManager is non-nil before
calling Get/Set which can panic in tests or alternate paths; guard these cache
accesses by checking if resource.CacheManager != nil before calling
GrpcDescriptorCache.Get or .Set, and if nil skip cache read/write and proceed
with the existing fallback behavior (i.e., continue to build/return the
descriptor source); ensure any early returns only happen when a cached value is
found and type-asserted successfully.
| os.Setenv(key, value) | ||
| slog.Debug("Set environment variable: %s=%s", key, value) | ||
| } |
There was a problem hiding this comment.
Handle os.Setenv errors and avoid logging secret values
- Check and log errors from os.Setenv.
- Don’t print raw values to logs.
Apply this diff:
- os.Setenv(key, value)
- slog.Debug("Set environment variable: %s=%s", key, value)
+ if err := os.Setenv(key, value); err != nil {
+ slog.Warn("Failed to set environment variable: %s, err=%v", key, err)
+ } else {
+ slog.Debug("Set environment variable: %s", key)
+ }- os.Setenv(key, value)
- slog.Debug("Loaded from file: %s=%s", key, value)
+ if err := os.Setenv(key, value); err != nil {
+ slog.Warn("Failed to set environment variable from file: %s, err=%v", key, err)
+ } else {
+ slog.Debug("Loaded from file: %s", key)
+ }- os.Setenv(key, value)
- slog.Debug("Set variable: %s=%s", key, value)
+ if err := os.Setenv(key, value); err != nil {
+ slog.Warn("Failed to set variable: %s, err=%v", key, err)
+ } else {
+ slog.Debug("Set variable: %s", key)
+ }Also applies to: 88-90, 99-101
🤖 Prompt for AI Agents
In internal/util/environment.go around lines 44-46, 88-90 and 99-101, the calls
to os.Setenv currently ignore the returned error and log raw values; change each
to capture the error (err := os.Setenv(key, value)) and if err != nil log it
with the key and err (e.g. slog.Error("failed to set env", "key", key, "err",
err)) or return/propagate the error as appropriate, and replace any debug logs
that print the raw value with a log that omits or redacts the value (e.g.
slog.Debug("set environment variable", "key", key, "value", "REDACTED") or only
log the key).
| // 然后从系统环境变量获取 | ||
| value := os.Getenv(key) | ||
| return value, value != "" | ||
| } |
There was a problem hiding this comment.
Use os.LookupEnv to distinguish empty values from unset
Current code treats empty string as “not found”.
Apply this diff:
- // 然后从系统环境变量获取
- value := os.Getenv(key)
- return value, value != ""
+ // 然后从系统环境变量获取
+ value, ok := os.LookupEnv(key)
+ return value, ok📝 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.
| // 然后从系统环境变量获取 | |
| value := os.Getenv(key) | |
| return value, value != "" | |
| } | |
| // 然后从系统环境变量获取 | |
| value, ok := os.LookupEnv(key) | |
| return value, ok | |
| } |
🤖 Prompt for AI Agents
In internal/util/environment.go around lines 110 to 113, the code uses os.Getenv
which cannot distinguish between an unset variable and one set to an empty
string; replace the os.Getenv call with os.LookupEnv, assign the returned
(value, ok) and return those so that an explicitly empty value is treated as
present (return "", true) while an unset variable returns ("", false).
internal/util/environment.go
Outdated
| file.WriteString("# AutoTest Environment Variables\n") | ||
| file.WriteString(fmt.Sprintf("# Generated at: %s\n\n", | ||
| fmt.Sprintf("%s", "2006-01-02 15:04:05"))) |
There was a problem hiding this comment.
Fix header writes: check errors, remove redundant Sprintf, and use real timestamp
Also addresses golang-ci errcheck/S1025 findings.
Apply this diff:
- file.WriteString("# AutoTest Environment Variables\n")
- file.WriteString(fmt.Sprintf("# Generated at: %s\n\n",
- fmt.Sprintf("%s", "2006-01-02 15:04:05")))
+ if _, err := fmt.Fprintln(file, "# AutoTest Environment Variables"); err != nil {
+ return fmt.Errorf("failed to write header: %w", err)
+ }
+ if _, err := fmt.Fprintf(file, "# Generated at: %s\n\n", time.Now().Format("2006-01-02 15:04:05")); err != nil {
+ return fmt.Errorf("failed to write generated-at header: %w", err)
+ }Add missing import:
// at the top import block
import (
"time"
)🧰 Tools
🪛 GitHub Actions: golang-ci
[error] 167-167: Error return value of file.WriteString is not checked (errcheck)
[error] 168-168: Error return value of file.WriteString is not checked (errcheck)
[error] 169-169: S1025: the argument is already a string, there's no need to use fmt.Sprintf (staticcheck)
🤖 Prompt for AI Agents
In internal/util/environment.go around lines 167 to 169, the header write
currently ignores write errors, uses a redundant fmt.Sprintf, and prints a
static timestamp string; add import "time" to the file, replace the redundant
fmt.Sprintf with direct formatting using time.Now().Format("2006-01-02
15:04:05") to generate a real timestamp, and check the error return from each
file.WriteString call (handle or return the error as appropriate) so we don't
ignore write failures and satisfy errcheck/S1025.
internal/util/report.go
Outdated
| } | ||
| defer file.Close() | ||
|
|
||
| file.WriteString(`<?xml version="1.0" encoding="UTF-8"?>` + "\n") |
There was a problem hiding this comment.
Check error from WriteString (fixes golang-ci errcheck)
Apply this diff:
- file.WriteString(`<?xml version="1.0" encoding="UTF-8"?>` + "\n")
+ if _, err := file.WriteString(`<?xml version="1.0" encoding="UTF-8"?>` + "\n"); err != nil {
+ return fmt.Errorf("failed to write XML header: %w", err)
+ }🧰 Tools
🪛 GitHub Actions: golang-ci
[error] 266-266: Error return value of file.WriteString is not checked (errcheck)
🤖 Prompt for AI Agents
In internal/util/report.go around line 266, the call to file.WriteString(`<?xml
version="1.0" encoding="UTF-8"?>` + "\n") ignores the returned values; capture
the returned _, err := file.WriteString(...) and handle the error (e.g., if err
!= nil { return err } or log and return as appropriate for the surrounding
function) so the write error is checked and propagated.
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/command/http_call.go (1)
174-304: Cache key ignores varying headers → wrong cached responsesLine 308 builds the cache key using only
method,url,Authorization, andContent-Type. Any other header (e.g. tenant, locale, feature flag) that influences the response is silently dropped, so distinct requests collapse to the same key. Once one variant is cached, every other variant receives that stale payload, producing incorrect test results. Please fold all request headers into the cache key deterministically (trim + sort before hashing) so unique header combinations yield unique cache entries.Apply this diff to harden the key generation:
+ "sort" @@ - key := fmt.Sprintf("%s:%s", method, url) - - // 包含重要的头信息 - for _, header := range headers { - if strings.Contains(strings.ToLower(header), "authorization") || - strings.Contains(strings.ToLower(header), "content-type") { - key += ":" + header - } - } - - return fmt.Sprintf("http:%x", md5.Sum([]byte(key))) + keyParts := make([]string, 0, len(headers)+2) + keyParts = append(keyParts, method, url) + + canonicalHeaders := make([]string, len(headers)) + for i, header := range headers { + canonicalHeaders[i] = strings.TrimSpace(header) + } + sort.Strings(canonicalHeaders) + keyParts = append(keyParts, canonicalHeaders...) + + return fmt.Sprintf("http:%x", md5.Sum([]byte(strings.Join(keyParts, "\n"))))
🧹 Nitpick comments (14)
.github/workflows/release.yml (2)
26-38: Inject version into the binary and harden cross-compiles.
- Inject tag into
main.versionvia-X.- Set
CGO_ENABLED=0for portable cross-builds.Apply:
- name: Build binaries run: | + set -euo pipefail + VERSION="${GITHUB_REF_NAME}" + LDFLAGS="-s -w -X main.version=${VERSION}" + export CGO_ENABLED=0 # Linux - GOOS=linux GOARCH=amd64 go build -ldflags "-s -w" -o autotest-linux-amd64 ./ - GOOS=linux GOARCH=arm64 go build -ldflags "-s -w" -o autotest-linux-arm64 ./ + GOOS=linux GOARCH=amd64 go build -ldflags "${LDFLAGS}" -o autotest-linux-amd64 ./ + GOOS=linux GOARCH=arm64 go build -ldflags "${LDFLAGS}" -o autotest-linux-arm64 ./ # macOS - GOOS=darwin GOARCH=amd64 go build -ldflags "-s -w" -o autotest-darwin-amd64 ./ - GOOS=darwin GOARCH=arm64 go build -ldflags "-s -w" -o autotest-darwin-arm64 ./ + GOOS=darwin GOARCH=amd64 go build -ldflags "${LDFLAGS}" -o autotest-darwin-amd64 ./ + GOOS=darwin GOARCH=arm64 go build -ldflags "${LDFLAGS}" -o autotest-darwin-arm64 ./ # Windows - GOOS=windows GOARCH=amd64 go build -ldflags "-s -w" -o autotest-windows-amd64.exe ./ + GOOS=windows GOARCH=amd64 go build -ldflags "${LDFLAGS}" -o autotest-windows-amd64.exe ./
43-51: Avoid attaching coverage HTML to releases; keep it as a workflow artifact.Attach binaries/checksums to releases; upload
coverage.htmlviaactions/upload-artifactinstead.Apply:
- name: Create Release uses: softprops/action-gh-release@v2 with: files: | autotest-* checksums.txt - coverage.html generate_release_notes: true + - name: Upload coverage report + uses: actions/upload-artifact@v4 + with: + name: coverage + path: coverage.htmlinternal/config/config.go (1)
16-35: Add config validation and document units/defaults.
- Enforce sane defaults for zero/negative values (e.g., Retry.MaxAttempts>=1, RetryDelay>0, Concurrency.MaxConcurrentRequests>=1, Cache.TTL>=0).
- Logger.Rotation MaxSize/MaxAge are strings; ensure parser handles units (e.g., “100MB”, “7d”) and document accepted formats.
Would you like a small
Validate()helper for AutoTestConfig.Global to apply defaults and return errors on invalid values?Also applies to: 36-45, 47-52, 53-60, 62-65
internal/util/retry.go (1)
26-34: Optional improvements: context, backoff/jitter, and removing unused field.
- Consider
ExecuteWithRetry(ctx context.Context, req *resty.Request)to allow canceling sleeps.- Add exponential backoff with jitter to reduce thundering herds.
RetryableHTTPClient.clientis unused; remove it or use it to construct the request.Also applies to: 69-74
internal/util/ratelimit.go (1)
64-88: Optional: acquire rate token before semaphore to reduce slot starvation.Acquire token first, then semaphore, so concurrent slots aren’t held idle while waiting for a token.
Also applies to: 100-108
internal/command/command.go (1)
285-303: Add safety to type assertions.The type assertions on lines 295-298 will panic if the cache stats map contains unexpected types. Since
GetStats()is an internal API under your control, this may be acceptable, but defensive coding is safer.Consider adding type checks:
func outputCacheStats() { if resource.CacheManager == nil { return } stats := resource.CacheManager.GetStats() slog.Info("=== Cache Statistics ===") for name, stat := range stats { - hits := stat["hits"].(int64) - misses := stat["misses"].(int64) - hitRate := stat["hit_rate"].(float64) - size := stat["size"].(int) + hits, ok := stat["hits"].(int64) + if !ok { + slog.Warn("Invalid hits type for cache %s", name) + continue + } + misses, ok := stat["misses"].(int64) + if !ok { + slog.Warn("Invalid misses type for cache %s", name) + continue + } + hitRate, ok := stat["hit_rate"].(float64) + if !ok { + slog.Warn("Invalid hit_rate type for cache %s", name) + continue + } + size, ok := stat["size"].(int) + if !ok { + slog.Warn("Invalid size type for cache %s", name) + continue + } slog.Info("Cache [%s]: Hits=%d, Misses=%d, HitRate=%.2f%%, Size=%d", name, hits, misses, hitRate, size) } }internal/luavm/init.go (1)
10-23: Consider a channel-based pool for deterministic Close() of LStates.sync.Pool may drop entries at GC without a chance to L.Close(); you can leak memory and can’t shut down cleanly. A bounded chan *lua.LState pool lets you Close() states on process shutdown.
I can sketch a small channel-backed pool with New/Get/Put/Close if desired. Based on learnings
internal/util/cache.go (2)
200-224: Allow stopping the background cleanup goroutine.cleanup() runs indefinitely; no stop/control path. This complicates tests/shutdown.
Add a context or done chan to Cache and stop ticker + exit on Close(). Also expose a Close() on CacheManager that calls each cache’s Close().
245-267: Expose “enabled” in stats to avoid guessing in callers.GetStats omits whether each cache is enabled; downstream code currently hardcodes Enabled=true.
Include enabled and ttl/maxSize in each stats entry:
- enabled: bool
- ttl_ms: int64
- max_size: int
This avoids fragile type assertions in consumers.internal/util/report.go (2)
195-203: Handle csv.Writer flush error.writer.Flush() may buffer errors; check writer.Error() after flushing.
writer := csv.NewWriter(file) defer writer.Flush() @@ if err := writer.Write(headers); err != nil { return fmt.Errorf("failed to write CSV headers: %w", err) } + // ... + // After all writes: + writer.Flush() + if err := writer.Error(); err != nil { + return fmt.Errorf("failed to flush CSV writer: %w", err) + }
110-134: Consider propagating errors instead of only logging per-format failures.Currently errors per format are logged and ignored, so GenerateReports returns nil even if all formats fail.
Aggregate errors (multi-error) or fail fast depending on your needs; at minimum return an error if no report was generated.
internal/util/cache_usage_example.go (3)
125-132: Normalize headers order in HTTP cache key.Headers slice order affects the key and hit rate. Sort it before processing.
- for _, header := range headers { + sorted := append([]string(nil), headers...) + sort.Strings(sorted) + for _, header := range sorted { if strings.Contains(strings.ToLower(header), "authorization") || strings.Contains(strings.ToLower(header), "content-type") { key += ":" + header } }Also import sort (see previous comment).
121-134: Consider avoiding raw Authorization header values in cache key input.Even though you hash the final key, you still keep the raw values in memory. Prefer hashing/salting auth values before concatenation or keying on the presence of auth vs. exact token, depending on semantics.
If per-user variations are required, hash only the token value and not the whole header string.
163-180: Fragile type assertions in GetCacheReport; rely on typed stats or include “enabled”.stat[...] assertions will panic if stats schema changes. Better: extend CacheManager.GetStats to include typed fields or expose methods, then avoid interface{} maps here.
If keeping current shape, guard with type switches and defaults to avoid panics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
.github/workflows/release.yml(1 hunks)config_files/autotest_enhanced.yml(1 hunks)doc/cache_guide.md(1 hunks)doc/usage.md(1 hunks)internal/command/command.go(3 hunks)internal/command/grpc_call.go(5 hunks)internal/command/http_call.go(4 hunks)internal/command/validation_helper.go(1 hunks)internal/command/validation_helper_test.go(1 hunks)internal/config/config.go(2 hunks)internal/luavm/init.go(1 hunks)internal/resource/resource.go(3 hunks)internal/rule/grpc_lua_rule.go(1 hunks)internal/rule/http_lua_rule.go(2 hunks)internal/util/cache.go(1 hunks)internal/util/cache_usage_example.go(1 hunks)internal/util/environment.go(1 hunks)internal/util/notification.go(1 hunks)internal/util/ratelimit.go(1 hunks)internal/util/report.go(1 hunks)internal/util/retry.go(1 hunks)main.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (15)
internal/rule/grpc_lua_rule.go (1)
internal/luavm/init.go (1)
ExecuteLuaWithGlobalsPool(94-136)
internal/command/command.go (2)
internal/resource/resource.go (6)
GlobalConfig(26-26)InitRestyClient(55-63)InitCacheManager(50-53)HttpTestCases(27-27)GrpcTestCases(28-28)CacheManager(37-37)internal/util/cache.go (1)
CacheManager(227-233)
internal/rule/http_lua_rule.go (1)
internal/luavm/init.go (1)
ExecuteLuaWithGlobalsPool(94-136)
internal/command/validation_helper_test.go (2)
internal/command/validation_helper.go (4)
ValidateTestCaseIDs(12-24)ValidateDependencies(27-53)ValidateXPath(56-63)ValidateBodyFields(66-72)internal/command/command.go (3)
ErrorIDduplicate(19-19)ErrorDependencyNotExist(20-20)ErrorIDRestrict(21-21)
internal/command/grpc_call.go (4)
internal/luavm/init.go (1)
ExecuteLuaWithGlobalsPool(94-136)internal/resource/resource.go (2)
CacheManager(37-37)DescSourceCache(36-36)internal/util/cache.go (1)
CacheManager(227-233)internal/model/model.go (1)
DescSourceCache(73-76)
internal/util/environment.go (1)
internal/config/config.go (1)
AutoTestConfig(9-65)
internal/util/cache.go (2)
internal/config/config.go (1)
AutoTestConfig(9-65)internal/resource/resource.go (1)
CacheManager(37-37)
internal/command/validation_helper.go (2)
internal/model/model.go (1)
IdItem(63-65)internal/command/command.go (3)
ErrorIDduplicate(19-19)ErrorDependencyNotExist(20-20)ErrorIDRestrict(21-21)
internal/util/retry.go (1)
internal/config/config.go (1)
AutoTestConfig(9-65)
internal/util/notification.go (1)
internal/config/config.go (1)
AutoTestConfig(9-65)
internal/util/report.go (1)
internal/config/config.go (1)
AutoTestConfig(9-65)
internal/util/cache_usage_example.go (2)
internal/util/cache.go (2)
Cache(23-33)CacheManager(227-233)internal/resource/resource.go (1)
CacheManager(37-37)
internal/resource/resource.go (1)
internal/util/cache.go (2)
CacheManager(227-233)NewCacheManager(236-243)
internal/command/http_call.go (3)
internal/luavm/init.go (1)
ExecuteLuaWithGlobalsPool(94-136)internal/resource/resource.go (1)
CacheManager(37-37)internal/util/cache.go (1)
CacheManager(227-233)
internal/config/config.go (1)
internal/util/cache.go (1)
Cache(23-33)
🪛 actionlint (1.7.8)
.github/workflows/release.yml
17-17: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
44-44: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 GitHub Actions: golang-ci
internal/util/environment.go
[error] 167-167: Error return value of file.WriteString is not checked (errcheck)
[error] 168-168: Error return value of file.WriteString is not checked (errcheck)
[error] 169-169: S1025: the argument is already a string, there's no need to use fmt.Sprintf (staticcheck)
[error] 177-177: QF1012: Use fmt.Fprintf(...) instead of WriteString(fmt.Sprintf(...)) (staticcheck)
internal/luavm/init.go
[error] 65-65: Error return value of L.DoString is not checked (errcheck)
[error] 95-95: shadow: declaration of "L" shadows declaration at line 85 (govet)
[error] 120-120: ST1023: should omit type lua.LValue from declaration; it will be inferred from the right-hand side (staticcheck)
internal/util/report.go
[error] 266-266: Error return value of file.WriteString is not checked (errcheck)
🪛 LanguageTool
doc/cache_guide.md
[uncategorized] ~192-~192: 动词的修饰一般为‘形容词（副词）+地+动词’。您的意思是否是：重要"地"请求
Context: ... 缓存键设计 缓存系统会自动生成缓存键，包含： - 请求方法和URL - 重要的请求头（如Authorization） - 模板内容和变量 - Lua脚本内容 ...
(wb4)
🔇 Additional comments (9)
internal/util/ratelimit.go (1)
71-80: Handle Stop in Acquire to avoid indefinite blocks.After Stop, token generation halts; waiting on tokens can block forever if ctx isn’t canceled. Include stopCh.
[Suggest_recommended_refactor]
Apply:if rl.tokens != nil { select { case <-rl.tokens: // 获得令牌 + case <-rl.stopCh: + rl.semaphore.Release(1) + return context.Canceled case <-ctx.Done(): rl.semaphore.Release(1) return ctx.Err() } }internal/util/retry.go (1)
47-60: Verify logger formatting: ensure simplelog’sDebug/Info/Errormethods accept printf-style arguments; otherwise switch toDebugf/Infof/Errorf.internal/util/notification.go (1)
1-190: LGTM! Well-structured notification service.The notification service implementation is clean and follows good practices:
- Proper separation of concerns with dedicated types for Slack messages
- Conditional notification logic based on test outcomes
- Sensible truncation of failed cases (limited to 10 with remaining count)
- Appropriate HTTP client timeout (10 seconds for webhooks)
- Comprehensive error handling and logging
doc/usage.md (1)
1-223: Excellent documentation coverage.The usage guide is comprehensive and well-organized, covering all essential aspects:
- Installation methods
- Configuration examples
- Test case writing for both HTTP and gRPC
- Advanced features (dependencies, Lua, XPath)
- Best practices
- Troubleshooting guidance
internal/rule/http_lua_rule.go (1)
95-95: Correct migration to pooled Lua execution.The switch to
ExecuteLuaWithGlobalsPoolproperly aligns with the new Lua VM pool architecture introduced ininternal/luavm/init.go. This enables state reuse and reduces allocation overhead.internal/rule/grpc_lua_rule.go (1)
89-89: Correct migration to pooled Lua execution.The switch to
ExecuteLuaWithGlobalsPoolproperly aligns with the new Lua VM pool architecture, matching the change ininternal/rule/http_lua_rule.go.doc/cache_guide.md (1)
1-253: Comprehensive cache system documentation.The cache guide thoroughly documents:
- Four cache types (gRPC descriptors, HTTP responses, templates, Lua scripts)
- Configuration options with clear explanations
- Performance tuning recommendations
- Best practices for cache key design and memory management
- Troubleshooting guidance with practical examples
This documentation effectively complements the cache implementation in
internal/util/cache.goand helps users leverage the caching system.internal/command/grpc_call.go (2)
280-319: Well-designed cache migration strategy.The implementation correctly integrates the new
GrpcDescriptorCachewhile maintaining backward compatibility with the existingDescSourceCache:
- Priority: New cache is checked first (line 282)
- Fallback: Old cache serves as fallback with forward-sync (line 293)
- Dual-write: New descriptors are stored in both caches (lines 316-317)
This approach ensures a smooth transition while preserving existing functionality.
262-262: Correct migration to pooled Lua execution.The switch to
ExecuteLuaWithGlobalsPoolis consistent with the changes ininternal/rule/http_lua_rule.goandinternal/rule/grpc_lua_rule.go, completing the migration to pool-based Lua VM execution.
| uses: actions/setup-go@v4 | ||
| with: | ||
| go-version: '1.24' | ||
|
|
There was a problem hiding this comment.
Update to latest actions/setup-go to avoid runner/runtime issues.
Use v5 per current runners.
Apply:
- uses: actions/setup-go@v4
+ uses: actions/setup-go@v5📝 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.
| uses: actions/setup-go@v4 | |
| with: | |
| go-version: '1.24' | |
| uses: actions/setup-go@v5 | |
| with: | |
| go-version: '1.24' |
🧰 Tools
🪛 actionlint (1.7.8)
17-17: the runner of "actions/setup-go@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
.github/workflows/release.yml around lines 17 to 20: the workflow pins
actions/setup-go to v4 which can cause runner/runtime issues on current GitHub
runners — update the action to actions/setup-go@v5 (replace the uses value) and
keep the existing go-version: '1.24' unless you intentionally want to bump the
Go version; commit the change so the workflow uses the latest supported setup-go
action.
| - name: Create Release | ||
| uses: softprops/action-gh-release@v1 | ||
| with: |
There was a problem hiding this comment.
Update softprops/action-gh-release to v2.
v1 is deprecated/older; v2 is current and resolves runner warnings.
Apply:
- uses: softprops/action-gh-release@v1
+ uses: softprops/action-gh-release@v2📝 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.
| - name: Create Release | |
| uses: softprops/action-gh-release@v1 | |
| with: | |
| - name: Create Release | |
| uses: softprops/action-gh-release@v2 | |
| with: |
🧰 Tools
🪛 actionlint (1.7.8)
44-44: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🤖 Prompt for AI Agents
.github/workflows/release.yml lines 43-45: the workflow is using the deprecated
softprops/action-gh-release@v1; update the action reference to
softprops/action-gh-release@v2, run a quick validation of the action
inputs/outputs in the workflow (adjust any input names if v2 changed them), and
commit the updated workflow so the runner warnings are resolved.
| // 3. 运行垃圾回收 | ||
| L.DoString("collectgarbage('collect')") | ||
| } |
There was a problem hiding this comment.
Check error from collectgarbage call (errcheck).
L.DoString(...) return value is ignored. Handle it or use L.GC to avoid string eval.
Apply one of these:
Option A (simple, no new imports):
- L.DoString("collectgarbage('collect')")
+ if err := L.DoString("collectgarbage('collect')"); err != nil {
+ // best-effort GC; ignore error
+ }Option B (prefer API call, if available in your gopher-lua version):
- L.DoString("collectgarbage('collect')")
+ L.GC(lua.GCCollect, 0)Based on learnings
📝 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.
| // 3. 运行垃圾回收 | |
| L.DoString("collectgarbage('collect')") | |
| } | |
| // 3. 运行垃圾回收 | |
| if err := L.DoString("collectgarbage('collect')"); err != nil { | |
| // best-effort GC; ignore error | |
| } | |
| } |
🧰 Tools
🪛 GitHub Actions: golang-ci
[error] 65-65: Error return value of L.DoString is not checked (errcheck)
🤖 Prompt for AI Agents
In internal/luavm/init.go around lines 64-66, the call
L.DoString("collectgarbage('collect')") ignores its returned error; either check
and handle that error or replace the string eval with the API GC call: if your
gopher-lua version exposes L.GC, call it directly (and handle its error/return)
to avoid string evaluation, otherwise capture the error from L.DoString,
propagate or log it appropriately (return the error from the enclosing function
or log and continue) so the failure is not silently dropped.
| L := GlobalLuaVMPool.GetLuaState() | ||
| defer GlobalLuaVMPool.PutLuaState(L) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid shadowing global L; rename local state variable (govet).
Local L shadows global L (Line 85). Rename to ls and update uses.
-func ExecuteLuaWithGlobalsPool(globals map[string]lua.LValue, source string) (lua.LValue, error) {
- L := GlobalLuaVMPool.GetLuaState()
- defer GlobalLuaVMPool.PutLuaState(L)
+func ExecuteLuaWithGlobalsPool(globals map[string]lua.LValue, source string) (lua.LValue, error) {
+ ls := GlobalLuaVMPool.GetLuaState()
+ defer GlobalLuaVMPool.PutLuaState(ls)
@@
- stackSizeBefore := L.GetTop()
+ stackSizeBefore := ls.GetTop()
@@
- L.SetGlobal(name, value)
+ ls.SetGlobal(name, value)
@@
- err := L.DoString(source)
+ err := ls.DoString(source)
@@
- L.SetTop(stackSizeBefore)
- cleanupGlobals(L, globalKeys)
+ ls.SetTop(stackSizeBefore)
+ cleanupGlobals(ls, globalKeys)
@@
- if L.GetTop() > stackSizeBefore {
- result = L.Get(-1)
+ if ls.GetTop() > stackSizeBefore {
+ result = ls.Get(-1)
// 对于复杂类型，创建副本以避免状态机回收后的引用问题
if result != lua.LNil {
- result = copyLuaValue(L, result)
+ result = copyLuaValue(ls, result)
}
}
@@
- L.SetTop(stackSizeBefore)
+ ls.SetTop(stackSizeBefore)
@@
- cleanupGlobals(L, globalKeys)
+ cleanupGlobals(ls, globalKeys)Also applies to: 101-109, 111-117, 121-127, 130-136
🧰 Tools
🪛 GitHub Actions: golang-ci
[error] 95-95: shadow: declaration of "L" shadows declaration at line 85 (govet)
🤖 Prompt for AI Agents
In internal/luavm/init.go around lines 95-97 (and similarly for ranges 101-109,
111-117, 121-127, 130-136), the local variable L shadows a global L; rename the
local Lua state variable to ls (or another non-conflicting name) and update all
subsequent uses and the defer call to use ls consistently; ensure imports/types
remain correct and run go vet/go build to confirm the shadowing is resolved.
| // 获取结果（复制值，避免引用问题） | ||
| var result lua.LValue = lua.LNil | ||
| if L.GetTop() > stackSizeBefore { | ||
| result = L.Get(-1) | ||
| // 对于复杂类型，创建副本以避免状态机回收后的引用问题 | ||
| if result != lua.LNil { | ||
| result = copyLuaValue(L, result) | ||
| } | ||
| } | ||
|
|
||
| // 清理栈，恢复到执行前的大小 | ||
| L.SetTop(stackSizeBefore) | ||
|
|
||
| // 清理本次设置的全局变量 | ||
| cleanupGlobals(L, globalKeys) | ||
|
|
||
| return result, nil | ||
| } |
There was a problem hiding this comment.
Returning lua.LValue from a pooled LState is unsafe; LTable copy still ties to recycled state.
copyLuaValue creates new tables in the same LState and returns them as lua.LValue. After PutLuaState, those values become invalid; future reuse/GC can corrupt consumers. Scalars (LString/LNumber/LBool) are fine; tables/functions/userdata are not.
Recommended fixes (pick one):
- Change ExecuteLuaWithGlobalsPool to return a Go value (interface{}) by deep‑converting LValues (maps/slices of Go scalars). Keep ExecuteLuaWithGlobals for legacy lua.LValue use.
- Or accept a callback: ExecuteLuaWithGlobalsPool(globals, source, func(L *lua.LState, result lua.LValue) error) so consumers use result before the state is returned.
- If you must keep the signature now, serialize complex results (tables) to a plain string (e.g., JSON) and return lua.LString, avoiding LTable entirely.
I can provide a concrete toGoValue implementation or a callback-based API if you prefer. Based on learnings
Also applies to: 145-165
🧰 Tools
🪛 GitHub Actions: golang-ci
[error] 120-120: ST1023: should omit type lua.LValue from declaration; it will be inferred from the right-hand side (staticcheck)
🤖 Prompt for AI Agents
internal/luavm/init.go around lines 119-136 (and similarly 145-165): returning
lua.LValue from a pooled LState is unsafe because copyLuaValue creates
LTable/LValue tied to the LState that will be returned to the pool; change the
API to accept a callback so the caller consumes the LValue before the state is
returned. Modify ExecuteLuaWithGlobalsPool signature to take an extra callback
param func(L *lua.LState, result lua.LValue) error (or similar), invoke the
callback immediately after obtaining/copying the result and before calling
PutLuaState/cleanupGlobals and SetTop, propagate callback errors, and ensure the
function returns nil lua.LValue (or change return type to error) so no LValue
escapes the pooled state; alternatively (if keeping return signature), convert
complex results to plain Go values (deep-convert tables to maps/slices of
scalars) or serialize tables to strings before releasing the state — implement
one of these fixes consistently at both code locations.
internal/util/environment.go
Outdated
| file.WriteString("# AutoTest Environment Variables\n") | ||
| file.WriteString(fmt.Sprintf("# Generated at: %s\n\n", | ||
| fmt.Sprintf("%s", "2006-01-02 15:04:05"))) |
There was a problem hiding this comment.
Fix error handling and timestamp generation.
Three issues need correction:
- Unchecked errors: Lines 167-168 don't check
WriteStringreturn values - Invalid timestamp: Line 169 uses the format string
"2006-01-02 15:04:05"as a literal instead of formatting the current time - Unnecessary wrapper: Line 169 has redundant
fmt.Sprintf("%s", ...)wrapper
Apply this diff to fix all three issues:
- file.WriteString("# AutoTest Environment Variables\n")
- file.WriteString(fmt.Sprintf("# Generated at: %s\n",
- fmt.Sprintf("%s", "2006-01-02 15:04:05")))
+ if _, err := file.WriteString("# AutoTest Environment Variables\n"); err != nil {
+ return fmt.Errorf("failed to write header: %w", err)
+ }
+ if _, err := file.WriteString(fmt.Sprintf("# Generated at: %s\n",
+ time.Now().Format("2006-01-02 15:04:05"))); err != nil {
+ return fmt.Errorf("failed to write timestamp: %w", err)
+ }📝 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.
| file.WriteString("# AutoTest Environment Variables\n") | |
| file.WriteString(fmt.Sprintf("# Generated at: %s\n\n", | |
| fmt.Sprintf("%s", "2006-01-02 15:04:05"))) | |
| if _, err := file.WriteString("# AutoTest Environment Variables\n"); err != nil { | |
| return fmt.Errorf("failed to write header: %w", err) | |
| } | |
| if _, err := file.WriteString(fmt.Sprintf("# Generated at: %s\n", | |
| time.Now().Format("2006-01-02 15:04:05"))); err != nil { | |
| return fmt.Errorf("failed to write timestamp: %w", err) | |
| } |
🧰 Tools
🪛 GitHub Actions: golang-ci
[error] 167-167: Error return value of file.WriteString is not checked (errcheck)
[error] 168-168: Error return value of file.WriteString is not checked (errcheck)
[error] 169-169: S1025: the argument is already a string, there's no need to use fmt.Sprintf (staticcheck)
🤖 Prompt for AI Agents
In internal/util/environment.go around lines 167 to 169, the two
file.WriteString calls currently ignore their returned (n, err) values, the
timestamp is using the layout string as a literal, and there is an unnecessary
fmt.Sprintf("%s", ...) wrapper; replace each WriteString call with captures like
_, err := file.WriteString(...); if err != nil { return err } (or otherwise
handle/log and return) and for the timestamp use time.Now().Format("2006-01-02
15:04:05") directly (no extra fmt.Sprintf) to write the generated-at line.
| func NewRateLimiter(maxConcurrent int, ratePerSecond int) *RateLimiter { | ||
| rl := &RateLimiter{ | ||
| semaphore: semaphore.NewWeighted(int64(maxConcurrent)), | ||
| stopCh: make(chan struct{}), | ||
| } | ||
|
|
||
| if ratePerSecond > 0 { | ||
| rl.tokens = make(chan struct{}, ratePerSecond) | ||
| rl.ticker = time.NewTicker(time.Second / time.Duration(ratePerSecond)) |
There was a problem hiding this comment.
Guard invalid maxConcurrent to avoid deadlocks.
Zero/negative values create a semaphore that can’t acquire. Default to 1.
Apply:
func NewRateLimiter(maxConcurrent int, ratePerSecond int) *RateLimiter {
+ if maxConcurrent <= 0 {
+ maxConcurrent = 1
+ }
rl := &RateLimiter{
semaphore: semaphore.NewWeighted(int64(maxConcurrent)),
stopCh: make(chan struct{}),
}📝 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.
| func NewRateLimiter(maxConcurrent int, ratePerSecond int) *RateLimiter { | |
| rl := &RateLimiter{ | |
| semaphore: semaphore.NewWeighted(int64(maxConcurrent)), | |
| stopCh: make(chan struct{}), | |
| } | |
| if ratePerSecond > 0 { | |
| rl.tokens = make(chan struct{}, ratePerSecond) | |
| rl.ticker = time.NewTicker(time.Second / time.Duration(ratePerSecond)) | |
| func NewRateLimiter(maxConcurrent int, ratePerSecond int) *RateLimiter { | |
| if maxConcurrent <= 0 { | |
| maxConcurrent = 1 | |
| } | |
| rl := &RateLimiter{ | |
| semaphore: semaphore.NewWeighted(int64(maxConcurrent)), | |
| stopCh: make(chan struct{}), | |
| } | |
| if ratePerSecond > 0 { | |
| rl.tokens = make(chan struct{}, ratePerSecond) | |
| rl.ticker = time.NewTicker(time.Second / time.Duration(ratePerSecond)) |
🤖 Prompt for AI Agents
In internal/util/ratelimit.go around lines 23 to 31, NewRateLimiter currently
constructs a semaphore with the provided maxConcurrent which if zero or negative
will create an unusable semaphore and can deadlock; guard against invalid values
by checking if maxConcurrent <= 0 and defaulting it to 1 before creating
semaphore (optionally record or log that you coerced the value), then proceed to
create the semaphore and rest of the RateLimiter as before.
internal/util/report.go
Outdated
| file.WriteString(`<?xml version="1.0" encoding="UTF-8"?>` + "\n") | ||
| encoder := xml.NewEncoder(file) | ||
| encoder.Indent("", " ") | ||
| if err := encoder.Encode(testSuite); err != nil { | ||
| return fmt.Errorf("failed to encode JUnit report: %w", err) | ||
| } |
There was a problem hiding this comment.
Check error from WriteString and return on failure (errcheck).
The XML header write ignores error.
- file.WriteString(`<?xml version="1.0" encoding="UTF-8"?>` + "\n")
+ if _, err := file.WriteString(`<?xml version="1.0" encoding="UTF-8"?>` + "\n"); err != nil {
+ return fmt.Errorf("failed to write XML header: %w", err)
+ }📝 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.
| file.WriteString(`<?xml version="1.0" encoding="UTF-8"?>` + "\n") | |
| encoder := xml.NewEncoder(file) | |
| encoder.Indent("", " ") | |
| if err := encoder.Encode(testSuite); err != nil { | |
| return fmt.Errorf("failed to encode JUnit report: %w", err) | |
| } | |
| if _, err := file.WriteString(`<?xml version="1.0" encoding="UTF-8"?>` + "\n"); err != nil { | |
| return fmt.Errorf("failed to write XML header: %w", err) | |
| } | |
| encoder := xml.NewEncoder(file) | |
| encoder.Indent("", " ") | |
| if err := encoder.Encode(testSuite); err != nil { | |
| return fmt.Errorf("failed to encode JUnit report: %w", err) | |
| } |
🧰 Tools
🪛 GitHub Actions: golang-ci
[error] 266-266: Error return value of file.WriteString is not checked (errcheck)
🤖 Prompt for AI Agents
In internal/util/report.go around lines 266 to 271, the call to file.WriteString
for the XML header currently ignores its error; capture the returned (n, )
error, check it, and return a wrapped fmt.Errorf on failure (e.g., "failed to
write XML header: %w") before continuing to create the encoder and encode the
testSuite.
| if lastErr == nil { | ||
| // 检查状态码是否需要重试 | ||
| if len(retryStatusCodes) == 0 || !retryStatusCodes[resp.StatusCode()] { | ||
| slog.Debug("Request succeeded on attempt %d", attempt) | ||
| return resp, nil | ||
| } | ||
| slog.Info("Status code %d requires retry, attempt %d/%d", resp.StatusCode(), attempt, maxAttempts) |
There was a problem hiding this comment.
Bug: default path never retries on 5xx; IsRetryableStatusCode is unused.
When no codes are configured, the current condition returns on any status. Use the default retryable set.
Apply:
- // 检查状态码是否需要重试
- if len(retryStatusCodes) == 0 || !retryStatusCodes[resp.StatusCode()] {
- slog.Debug("Request succeeded on attempt %d", attempt)
- return resp, nil
- }
+ // 检查状态码是否需要重试
+ if len(retryStatusCodes) == 0 {
+ if !IsRetryableStatusCode(resp.StatusCode(), nil) {
+ slog.Debug("Request succeeded on attempt %d", attempt)
+ return resp, nil
+ }
+ } else if !retryStatusCodes[resp.StatusCode()] {
+ slog.Debug("Request succeeded on attempt %d", attempt)
+ return resp, nil
+ }
slog.Info("Status code %d requires retry, attempt %d/%d", resp.StatusCode(), attempt, maxAttempts)📝 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.
| if lastErr == nil { | |
| // 检查状态码是否需要重试 | |
| if len(retryStatusCodes) == 0 || !retryStatusCodes[resp.StatusCode()] { | |
| slog.Debug("Request succeeded on attempt %d", attempt) | |
| return resp, nil | |
| } | |
| slog.Info("Status code %d requires retry, attempt %d/%d", resp.StatusCode(), attempt, maxAttempts) | |
| if lastErr == nil { | |
| // 检查状态码是否需要重试 | |
| if len(retryStatusCodes) == 0 { | |
| if !IsRetryableStatusCode(resp.StatusCode(), nil) { | |
| slog.Debug("Request succeeded on attempt %d", attempt) | |
| return resp, nil | |
| } | |
| } else if !retryStatusCodes[resp.StatusCode()] { | |
| slog.Debug("Request succeeded on attempt %d", attempt) | |
| return resp, nil | |
| } | |
| slog.Info("Status code %d requires retry, attempt %d/%d", resp.StatusCode(), attempt, maxAttempts) |
🤖 Prompt for AI Agents
In internal/util/retry.go around lines 51-57, the branch currently returns
success for any status when retryStatusCodes is empty, so the default 5xx retry
logic (IsRetryableStatusCode) is never used; change the logic so that when
len(retryStatusCodes) == 0 you call IsRetryableStatusCode(resp.StatusCode()) to
decide retryability, otherwise use retryStatusCodes[resp.StatusCode()]; i.e.
only return success when the applicable retry check (default
IsRetryableStatusCode for empty map, or the explicit map) indicates the status
is not retryable, and keep the existing logs but use the chosen check for the
"requires retry" path.
| &cli.StringFlag{Name: "environment", Aliases: []string{"env"}, Usage: "specify environment (dev/staging/prod)"}, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
Ensure the new --environment flag is actually consumed.
Confirm command.RunTestCases reads this flag and uses it to select env. Also consider clarifying the difference between --env-file and --env to avoid confusion.
Run:
🏁 Script executed:
#!/bin/bash
# Find reads of the new flag
rg -nP --type=go -C3 '\.String\("environment"\)|\.String\("env"\)'
# Find where RunTestCases accesses flags/context
rg -nP --type=go -C5 'RunTestCases'Length of output: 1395
---
🏁 Script executed:
```shell
#!/bin/bash
# Get complete RunTestCases function implementation
rg -nP --type=go -A 50 'func RunTestCases'
Length of output: 3257
Consume the --environment flag in RunTestCases
In internal/command/command.go (after envFilePath), add environment := cmd.String("environment") and apply it to select the target environment. Update the Usage text on both flags to clarify that --env-file is a file path and --env (alias of --environment) chooses among dev/staging/prod.
🤖 Prompt for AI Agents
In main.go around lines 45-46, the CLI defines an "environment" flag but the
command handler doesn't consume it; in internal/command/command.go after
retrieving envFilePath add environment := cmd.String("environment") (using the
same flag name/alias) and wire that value into RunTestCases (or the routine that
selects the target environment) so the chosen env ("dev"/"staging"/"prod")
controls environment selection; also update the Usage strings for both
--env-file and --environment/--env to state that --env-file expects a file path
and --environment (alias --env) selects one of dev/staging/prod.
Summary by CodeRabbit
New Features
Performance
Documentation
Tests
Chores