refactor: refactor config to use constants for default values #845
Conversation
- Define configuration defaults as constants for improved maintainability - Use constants for setting viper default values throughout the config - Ensure runtime.NumCPU is utilized for worker number when configured as zero - Use DefaultQueueNum for queue number when configured as zero - Refactor tests to verify default values using constants instead of hardcoded numbers - Improve configurability and ease of updating default settings Signed-off-by: appleboy <[email protected]>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds exported Default* constants, runtime fallback logic for worker/queue counts, and validation helpers (port/address/pid/config); updates setDefaults to use the constants and adjusts tests to assert new defaults and cover negative/zero worker/queue boundary behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App Startup
participant Viper as Viper Loader
participant Config as config/config.go
participant OS as runtime
App->>Viper: Load raw config (ConfYaml)
Viper-->>App: ConfYaml (may include zero-values)
App->>Config: setDefaults(cfg)
Note right of Config #eef6ff: Apply exported Default* constants where fields are empty
Config-->>App: cfg (defaults applied)
App->>Config: loadConfigFromViper(cfg)
alt Core.WorkerNum <= 0
Config->>OS: runtime.NumCPU()
OS-->>Config: cpuCount
Config-->>App: Core.WorkerNum = cpuCount
end
alt Core.QueueNum <= 0
Config-->>App: Core.QueueNum = DefaultQueueNum
end
App->>Config: ValidateConfig(cfg)
Config->>Config: ValidatePort / ValidateAddress / ValidatePIDPath
Config-->>App: validation result (error or success)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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.
Summary of Changes
Hello @appleboy, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request refactors the application's configuration management by replacing hardcoded default values with named constants. This change significantly enhances the maintainability and clarity of the configuration, making it easier to understand and update default settings in the future. It also standardizes how dynamic defaults, like worker and queue numbers, are applied.
Highlights
- Configuration Constants: Introduced a set of constants (e.g., DefaultWorkerNum, DefaultQueueNum, DefaultPort) to centralize and define default configuration values, improving readability and maintainability.
- Default Value Assignment: Replaced hardcoded default values with the newly defined constants across the configuration setup using the viper library, ensuring consistency.
- Dynamic Worker/Queue Sizing: Ensured that runtime.NumCPU() is utilized for the worker number and DefaultQueueNum for the queue number when their configured values are zero, optimizing resource utilization based on system capabilities.
- Test Refactoring: Updated existing tests to assert against the new configuration constants instead of hardcoded numbers, improving test maintainability and consistency.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request is a great step towards improving the codebase's maintainability by replacing hardcoded default configuration values with named constants. The changes are consistently applied across the application code and tests. I've pointed out a minor issue where a constant's name doesn't match its semantic use, which could be confusing. Addressing this will make the code even clearer.
| viper.SetDefault("core.shutdown_timeout", DefaultShutdownTimeout) | ||
| viper.SetDefault("core.port", DefaultPort) | ||
| viper.SetDefault("core.worker_num", DefaultWorkerNum) | ||
| viper.SetDefault("core.queue_num", DefaultWorkerNum) // 0, will be set to DefaultQueueNum at runtime if still 0 |
There was a problem hiding this comment.
Using DefaultWorkerNum for setting the default value of core.queue_num is confusing. While it currently works because DefaultWorkerNum is 0, this creates a hidden dependency between the two settings. If DefaultWorkerNum were to change in the future, it would unintentionally affect queue_num. For better clarity and maintainability, it's recommended to use the literal 0 here.
| viper.SetDefault("core.queue_num", DefaultWorkerNum) // 0, will be set to DefaultQueueNum at runtime if still 0 | |
| viper.SetDefault("core.queue_num", 0) // 0, will be set to DefaultQueueNum at runtime if still 0 |
config/config.go
Outdated
|
|
||
| if conf.Core.QueueNum == int64(0) { | ||
| conf.Core.QueueNum = int64(8192) | ||
| if conf.Core.QueueNum == DefaultWorkerNum { |
There was a problem hiding this comment.
Using DefaultWorkerNum to check the value of conf.Core.QueueNum is confusing for the same reasons as in setDefaults. It makes the code harder to understand and more brittle. Please use the literal 0 for this comparison to make the intent clear.
| if conf.Core.QueueNum == DefaultWorkerNum { | |
| if conf.Core.QueueNum == 0 { |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/config_test.go (1)
302-306: Fix global state leak: defaultConf not restored → order-dependent failures.This test mutates a package-level var without restoring it. Use the same backup/restore pattern as TestDefaultConfigLoadFailure.
func TestLoadWrongDefaultYAMLConfig(t *testing.T) { - defaultConf = []byte(`a`) - _, err := LoadConf() - assert.Error(t, err) + original := defaultConf + defer func() { defaultConf = original }() + defaultConf = []byte(`a`) + _, err := LoadConf() + assert.Error(t, err) }
🧹 Nitpick comments (5)
config/config.go (3)
331-336: Avoid conflating queue_num default with worker_num sentinel.Setting core.queue_num’s default via DefaultWorkerNum (0) is confusing. Default to DefaultQueueNum directly; keep the runtime fallback to honor explicit 0 in config.
- viper.SetDefault("core.queue_num", DefaultWorkerNum) // 0, will be set to DefaultQueueNum at runtime if still 0 + viper.SetDefault("core.queue_num", DefaultQueueNum) // use canonical default; explicit 0 in config still handled at runtime
563-587: Overly strict PID path restrictions; allow common runtime dirs.Blocking all of /var prevents standard locations like /var/run or /run. Consider allowlisting these while keeping the rest blocked.
func ValidatePIDPath(pidPath string) error { ... - // Ensure the path is not trying to write to sensitive system directories - sensitive := []string{"/etc/", "/usr/", "/var/", "/sys/", "/proc/"} + // Allow common runtime pid locations explicitly + allowed := []string{"/run/", "/var/run/"} + for _, a := range allowed { + if strings.HasPrefix(cleanPath, a) { + return nil + } + } + // Block sensitive system directories + sensitive := []string{"/etc/", "/usr/", "/var/", "/sys/", "/proc/"} for _, prefix := range sensitive { if strings.HasPrefix(cleanPath, prefix) { return fmt.Errorf("cannot write PID file to sensitive directory: %s", cleanPath) } }
589-618: Add a few more validations (gRPC port, queue redis addr, and non-negative sizes).Strengthen ValidateConfig to catch bad inputs earlier.
func ValidateConfig(cfg *ConfYaml) error { if err := ValidatePort(cfg.Core.Port); err != nil { return fmt.Errorf("invalid core port: %w", err) } + if cfg.GRPC.Port != "" { + if err := ValidatePort(cfg.GRPC.Port); err != nil { + return fmt.Errorf("invalid gRPC port: %w", err) + } + } if err := ValidateAddress(cfg.Core.Address); err != nil { return fmt.Errorf("invalid core address: %w", err) } if err := ValidatePIDPath(cfg.Core.PID.Path); err != nil { return fmt.Errorf("invalid PID path: %w", err) } // Validate Redis address if Redis is enabled if cfg.Stat.Engine == "redis" && cfg.Stat.Redis.Addr != "" { host, port, err := net.SplitHostPort(cfg.Stat.Redis.Addr) if err != nil { return fmt.Errorf("invalid Redis address format: %s", cfg.Stat.Redis.Addr) } if err := ValidateAddress(host); err != nil { return fmt.Errorf("invalid Redis host: %w", err) } if err := ValidatePort(port); err != nil { return fmt.Errorf("invalid Redis port: %w", err) } } + // Validate queue redis address if queue engine is redis + if cfg.Queue.Engine == "redis" && cfg.Queue.Redis.Addr != "" { + host, port, err := net.SplitHostPort(cfg.Queue.Redis.Addr) + if err != nil { + return fmt.Errorf("invalid queue Redis address format: %s", cfg.Queue.Redis.Addr) + } + if err := ValidateAddress(host); err != nil { + return fmt.Errorf("invalid queue Redis host: %w", err) + } + if err := ValidatePort(port); err != nil { + return fmt.Errorf("invalid queue Redis port: %w", err) + } + } + // Sanity checks for sizes + if cfg.Core.WorkerNum < 1 { + return fmt.Errorf("worker_num must be >= 1 (got %d)", cfg.Core.WorkerNum) + } + if cfg.Core.QueueNum < 1 { + return fmt.Errorf("queue_num must be >= 1 (got %d)", cfg.Core.QueueNum) + } return nil }config/config_test.go (2)
278-301: Prefer t.Setenv for isolation.Using os.Setenv without cleanup can bleed into other tests; t.Setenv auto-restores.
func TestLoadConfigFromEnv(t *testing.T) { - os.Setenv("GORUSH_CORE_PORT", "9001") - os.Setenv("GORUSH_GRPC_ENABLED", "true") - os.Setenv("GORUSH_CORE_MAX_NOTIFICATION", "200") - os.Setenv("GORUSH_IOS_KEY_ID", "ABC123DEFG") - os.Setenv("GORUSH_IOS_TEAM_ID", "DEF123GHIJ") - os.Setenv("GORUSH_API_HEALTH_URI", "/healthz") - os.Setenv("GORUSH_CORE_FEEDBACK_HOOK_URL", "http://example.com") - os.Setenv("GORUSH_CORE_FEEDBACK_HEADER", "x-api-key:1234567890 x-auth-key:0987654321") + t.Setenv("GORUSH_CORE_PORT", "9001") + t.Setenv("GORUSH_GRPC_ENABLED", "true") + t.Setenv("GORUSH_CORE_MAX_NOTIFICATION", "200") + t.Setenv("GORUSH_IOS_KEY_ID", "ABC123DEFG") + t.Setenv("GORUSH_IOS_TEAM_ID", "DEF123GHIJ") + t.Setenv("GORUSH_API_HEALTH_URI", "/healthz") + t.Setenv("GORUSH_CORE_FEEDBACK_HOOK_URL", "http://example.com") + t.Setenv("GORUSH_CORE_FEEDBACK_HEADER", "x-api-key:1234567890 x-auth-key:0987654321")
66-68: Use t.Fatalf instead of panic for failed fixture loads.Keeps test output structured and avoids stack dumps.
- if err != nil { - panic("failed to load config.yml from file") - } + if err != nil { + t.Fatalf("failed to load config.yml from file: %v", err) + }Apply similarly in SetupTest and TestLoadConfigFromEnv.
Also applies to: 82-88, 287-290
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/config.go(5 hunks)config/config_test.go(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
config/config_test.go (1)
config/config.go (8)
DefaultMaxConcurrentPush(25-25)DefaultPort(23-23)DefaultShutdownTimeout(21-21)DefaultQueueNum(19-19)DefaultFeedbackTimeout(22-22)DefaultMaxNotification(20-20)DefaultGRPCPort(24-24)ValidatePort(534-546)
config/config.go (1)
main.go (1)
main(32-409)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
- GitHub Check: build-docker
- GitHub Check: Analyze (go)
- GitHub Check: testing
🔇 Additional comments (6)
config/config.go (3)
16-26: Good: centralized, exported defaults.Exporting these constants improves maintainability and test readability.
533-546: Port validation looks solid.
548-561: Address validation is reasonable for our use.config/config_test.go (3)
70-70: LGTM: asserting iOS concurrency via constant.
94-99: LGTM: tests now reference exported defaults instead of literals.This reduces churn when defaults change.
Also applies to: 103-103, 109-109, 141-141, 189-189, 195-199, 202-202, 210-210, 242-242, 271-271
623-623: LGTM: integration test now tied to DefaultPort.
- Ensure WorkerNum and QueueNum are set to positive defaults if configured with zero or negative values - Prevent potential panics by treating non-positive configuration values as invalid and replacing them at runtime - Add tests to verify negative and zero configuration values are safely corrected to defaults - Add new safety boundary tests for QueueNum to further validate configuration handling Signed-off-by: appleboy <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/config.go (1)
603-615: Redis cluster addresses are rejected; handle comma-separated hostsValidateConfig uses net.SplitHostPort on the entire Addr, which fails for cluster lists like "host1:6379,host2:6380". Iterate and validate each when cluster is enabled.
- // Validate Redis address if Redis is enabled - if cfg.Stat.Engine == "redis" && cfg.Stat.Redis.Addr != "" { - host, port, err := net.SplitHostPort(cfg.Stat.Redis.Addr) - if err != nil { - return fmt.Errorf("invalid Redis address format: %s", cfg.Stat.Redis.Addr) - } - if err := ValidateAddress(host); err != nil { - return fmt.Errorf("invalid Redis host: %w", err) - } - if err := ValidatePort(port); err != nil { - return fmt.Errorf("invalid Redis port: %w", err) - } - } + // Validate Redis address(es) if Redis is used + if cfg.Stat.Engine == "redis" && strings.TrimSpace(cfg.Stat.Redis.Addr) != "" { + addrs := []string{cfg.Stat.Redis.Addr} + if cfg.Stat.Redis.Cluster { + addrs = strings.Split(cfg.Stat.Redis.Addr, ",") + } + for _, raw := range addrs { + raw = strings.TrimSpace(raw) + host, port, err := net.SplitHostPort(raw) + if err != nil { + return fmt.Errorf("invalid Redis address format: %s", raw) + } + if err := ValidateAddress(host); err != nil { + return fmt.Errorf("invalid Redis host: %w", err) + } + if err := ValidatePort(port); err != nil { + return fmt.Errorf("invalid Redis port: %w", err) + } + } + }
♻️ Duplicate comments (1)
config/config.go (1)
334-335: Use literal 0 for queue_num default to avoid semantic couplingSetting core.queue_num’s default via DefaultWorkerNum is confusing and brittle. Prefer 0 directly with an explanatory comment.
- viper.SetDefault("core.queue_num", DefaultWorkerNum) // 0, will be set to DefaultQueueNum at runtime if still 0 + viper.SetDefault("core.queue_num", 0) // 0, will be set to DefaultQueueNum at runtime if still 0
🧹 Nitpick comments (5)
config/config_test.go (2)
650-731: Avoid duplicating runtime-defaulting logic in testsThe test re-implements production fallback logic; this risks drift. Extract the fallback into a helper and call it here.
Apply in config/config.go:
@@ - // Apply runtime-computed defaults for zero or negative values - // This ensures optimal resource utilization and prevents panics from negative values - if conf.Core.WorkerNum <= 0 { - conf.Core.WorkerNum = int64(runtime.NumCPU()) - } - if conf.Core.QueueNum <= 0 { - conf.Core.QueueNum = DefaultQueueNum - } + // Apply runtime-computed defaults for zero or negative values + applyRuntimeDefaults(conf) return conf, nil } +// applyRuntimeDefaults normalizes non-positive configuration values. +func applyRuntimeDefaults(conf *ConfYaml) { + if conf.Core.WorkerNum <= 0 { + conf.Core.WorkerNum = int64(runtime.NumCPU()) + } + if conf.Core.QueueNum <= 0 { + conf.Core.QueueNum = DefaultQueueNum + } + if conf.Ios.MaxConcurrentPushes == 0 { + conf.Ios.MaxConcurrentPushes = DefaultMaxConcurrentPush + } +}Then simplify the test:
@@ - // Apply the same logic as loadConfigFromViper - if conf.Core.WorkerNum <= 0 { - conf.Core.WorkerNum = int64(runtime.NumCPU()) - } - if conf.Core.QueueNum <= 0 { - conf.Core.QueueNum = DefaultQueueNum - } + // Normalize using production helper + applyRuntimeDefaults(conf)
732-761: LGTM: queue safety boundary testsGood guard-rail checks on non-positive values. Consider adding a parallel worker_num boundary table here too (optional; already covered above).
config/config.go (3)
520-528: LGTM: non-positive guards for WorkerNum/QueueNumFixes the negative-values hole and ensures safe, predictable capacity. Consider also normalizing iOS MaxConcurrentPushes to a positive value to avoid unbuffered channels if configured as 0.
if conf.Core.QueueNum <= 0 { conf.Core.QueueNum = DefaultQueueNum } + + // Keep iOS gate positive; avoid unbuffered channel on 0 + if conf.Ios.MaxConcurrentPushes == 0 { + conf.Ios.MaxConcurrentPushes = DefaultMaxConcurrentPush + }
548-551: Fix ValidateAddress doc to reflect behaviorThe function allows empty address; update the comment for accuracy.
-// ValidateAddress validates that an address is not empty and contains valid characters +// ValidateAddress validates that an address, if provided, is a plausible IP or hostname.
589-596: Also validate gRPC port when enabledParity with Core.Port validation improves early failure for bad configs.
func ValidateConfig(cfg *ConfYaml) error { if err := ValidatePort(cfg.Core.Port); err != nil { return fmt.Errorf("invalid core port: %w", err) } + if cfg.GRPC.Enabled { + if err := ValidatePort(cfg.GRPC.Port); err != nil { + return fmt.Errorf("invalid gRPC port: %w", err) + } + } + if err := ValidateAddress(cfg.Core.Address); err != nil { return fmt.Errorf("invalid core address: %w", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/config.go(5 hunks)config/config_test.go(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
config/config.go (1)
main.go (1)
main(32-409)
config/config_test.go (1)
config/config.go (9)
DefaultMaxConcurrentPush(25-25)DefaultPort(23-23)DefaultShutdownTimeout(21-21)DefaultQueueNum(19-19)DefaultFeedbackTimeout(22-22)DefaultMaxNotification(20-20)DefaultGRPCPort(24-24)ValidatePort(534-546)ConfYaml(145-155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
- GitHub Check: Analyze (go)
- GitHub Check: build-docker
- GitHub Check: testing
🔇 Additional comments (8)
config/config_test.go (4)
70-70: LGTM: tests assert against canonical defaultsAsserting with DefaultMaxConcurrentPush improves maintainability.
94-99: LGTM: swap hardcoded values for exported defaultsUsing DefaultPort, DefaultShutdownTimeout, DefaultQueueNum, DefaultFeedbackTimeout, DefaultMaxNotification, DefaultMaxConcurrentPush, and DefaultGRPCPort removes magic numbers and aligns tests with config semantics.
Also applies to: 103-110, 141-141, 189-190
194-199: LGTM: test expectations updated to constantsGood coverage of core, iOS, and gRPC defaults; keeps tests resilient to future default changes.
Also applies to: 202-203, 210-211, 242-242, 271-272
623-624: LGTM: integration test uses DefaultPortThis avoids drift if the default port changes.
config/config.go (4)
16-26: LGTM: centralized, exported defaultsClear single-source-of-truth for defaults; comments document semantics (e.g., 0 → runtime.NumCPU()).
331-338: LGTM: defaults wired to constants across core settingsConsistent use of DefaultShutdownTimeout, DefaultPort, DefaultWorkerNum, DefaultMaxNotification, DefaultFeedbackTimeout reduces magic numbers.
352-353: LGTM: iOS concurrent pushes default via constantTyped cast to uint is appropriate.
361-361: LGTM: gRPC port default constantKeeps port defaults consistent across code/tests.
- Change temporary file permission mode from 0644 to 0600 when writing invalid YAML test content Signed-off-by: appleboy <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
config/config_test.go (2)
278-287: Isolate env overrides with t.Setenv to avoid test bleed-throughUsing os.Setenv leaks state across tests and orders. t.Setenv auto-restores.
Apply:
- os.Setenv("GORUSH_CORE_PORT", "9001") - os.Setenv("GORUSH_GRPC_ENABLED", "true") - os.Setenv("GORUSH_CORE_MAX_NOTIFICATION", "200") - os.Setenv("GORUSH_IOS_KEY_ID", "ABC123DEFG") - os.Setenv("GORUSH_IOS_TEAM_ID", "DEF123GHIJ") - os.Setenv("GORUSH_API_HEALTH_URI", "/healthz") - os.Setenv("GORUSH_CORE_FEEDBACK_HOOK_URL", "http://example.com") - os.Setenv("GORUSH_CORE_FEEDBACK_HEADER", "x-api-key:1234567890 x-auth-key:0987654321") + t.Setenv("GORUSH_CORE_PORT", "9001") + t.Setenv("GORUSH_GRPC_ENABLED", "true") + t.Setenv("GORUSH_CORE_MAX_NOTIFICATION", "200") + t.Setenv("GORUSH_IOS_KEY_ID", "ABC123DEFG") + t.Setenv("GORUSH_IOS_TEAM_ID", "DEF123GHIJ") + t.Setenv("GORUSH_API_HEALTH_URI", "/healthz") + t.Setenv("GORUSH_CORE_FEEDBACK_HOOK_URL", "http://example.com") + t.Setenv("GORUSH_CORE_FEEDBACK_HEADER", "x-api-key:1234567890 x-auth-key:0987654321")If older Go version is in use, manually capture and restore env.
302-306: Restore defaultConf after mutation to prevent cross-test flakinessThis test permanently corrupts the global default unless restored. Mirrors the pattern used in TestDefaultConfigLoadFailure.
Apply:
func TestLoadWrongDefaultYAMLConfig(t *testing.T) { - defaultConf = []byte(`a`) + orig := defaultConf + t.Cleanup(func() { defaultConf = orig }) + defaultConf = []byte(`a`) _, err := LoadConf() assert.Error(t, err) }
🧹 Nitpick comments (5)
config/config_test.go (5)
32-32: Prefer t.TempDir/t.CreateTemp over hard-coded temp file namesUsing a fixed filename can collide in parallel runs. Keep 0600, but create/delete via testing helpers for isolation.
Example:
dir := t.TempDir() tmpFile := filepath.Join(dir, "invalid.yml") err := os.WriteFile(tmpFile, content, 0o600)(Add
path/filepathto imports.)
94-109: LGTM: swapped literals for Default constants with correct typing*Assertions now bind to exported defaults; int64/uint casts look correct for struct field types.
Minor: prefer assert.True/False over assert.Equal(..., true/false) for readability.
Also applies to: 141-141, 189-189
194-210: LGTM: constant-driven expectations in TestValidateConfSame comments as above; type conversions are consistent.
Optional: use assert.True/False where applicable.
Also applies to: 271-271
733-761: Remove unused struct field and align with production pathThe usesCPU field is unused. Also consider driving this test via LoadConf + YAML like the previous comment to exercise real boundary handling.
Apply:
- tests := []struct { + tests := []struct { name string value int64 expected int64 - usesCPU bool }{ - {"very_negative", -9999, DefaultQueueNum, false}, - {"negative_one", -1, DefaultQueueNum, false}, - {"zero", 0, DefaultQueueNum, false}, - {"positive_one", 1, 1, false}, + {"very_negative", -9999, DefaultQueueNum}, + {"negative_one", -1, DefaultQueueNum}, + {"zero", 0, DefaultQueueNum}, + {"positive_one", 1, 1}, }
81-89: Use require in setup to fail fast instead of panicsuite.Require().NoError is clearer and integrates with test reporting.
Example:
suite.Require().NoError(err, "failed to load default config.yml")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
config/config_test.go(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
config/config_test.go (1)
config/config.go (9)
DefaultMaxConcurrentPush(25-25)DefaultPort(23-23)DefaultShutdownTimeout(21-21)DefaultQueueNum(19-19)DefaultFeedbackTimeout(22-22)DefaultMaxNotification(20-20)DefaultGRPCPort(24-24)ValidatePort(534-546)ConfYaml(145-155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
- GitHub Check: build-docker
- GitHub Check: Analyze (go)
- GitHub Check: testing
🔇 Additional comments (3)
config/config_test.go (3)
70-70: LGTM: assert now references DefaultMaxConcurrentPushGood move away from magic numbers.
559-559: LGTM: ValidateConfig test uses DefaultPortKeeps the test resilient to default updates.
623-623: LGTM: integration test ties to DefaultPortAvoids hard-coding and follows the refactor intent.
| // TestNegativeValues tests that negative configuration values are handled safely | ||
| func TestNegativeValues(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| workerNum int64 | ||
| queueNum int64 | ||
| expectedWorker int64 | ||
| expectedQueue int64 | ||
| description string | ||
| }{ | ||
| { | ||
| name: "negative_worker_num_should_use_cpu_count", | ||
| workerNum: -5, | ||
| queueNum: DefaultQueueNum, | ||
| expectedWorker: int64(runtime.NumCPU()), | ||
| expectedQueue: DefaultQueueNum, | ||
| description: "Negative WorkerNum should be replaced with CPU count", | ||
| }, | ||
| { | ||
| name: "negative_queue_num_should_use_default", | ||
| workerNum: 4, // Valid positive value | ||
| queueNum: -100, | ||
| expectedWorker: 4, | ||
| expectedQueue: DefaultQueueNum, | ||
| description: "Negative QueueNum should be replaced with default", | ||
| }, | ||
| { | ||
| name: "both_negative_should_use_defaults", | ||
| workerNum: -1, | ||
| queueNum: -1, | ||
| expectedWorker: int64(runtime.NumCPU()), | ||
| expectedQueue: DefaultQueueNum, | ||
| description: "Both negative values should be replaced with defaults", | ||
| }, | ||
| { | ||
| name: "zero_worker_num_should_use_cpu_count", | ||
| workerNum: 0, | ||
| queueNum: DefaultQueueNum, | ||
| expectedWorker: int64(runtime.NumCPU()), | ||
| expectedQueue: DefaultQueueNum, | ||
| description: "Zero WorkerNum should be replaced with CPU count", | ||
| }, | ||
| { | ||
| name: "zero_queue_num_should_use_default", | ||
| workerNum: 4, | ||
| queueNum: 0, | ||
| expectedWorker: 4, | ||
| expectedQueue: DefaultQueueNum, | ||
| description: "Zero QueueNum should be replaced with default", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| // Create config with test values | ||
| conf := &ConfYaml{} | ||
| conf.Core.WorkerNum = tt.workerNum | ||
| conf.Core.QueueNum = tt.queueNum | ||
|
|
||
| // Apply the same logic as loadConfigFromViper | ||
| if conf.Core.WorkerNum <= 0 { | ||
| conf.Core.WorkerNum = int64(runtime.NumCPU()) | ||
| } | ||
| if conf.Core.QueueNum <= 0 { | ||
| conf.Core.QueueNum = DefaultQueueNum | ||
| } | ||
|
|
||
| // Verify results | ||
| assert.Equal(t, tt.expectedWorker, conf.Core.WorkerNum, | ||
| "WorkerNum: %s", tt.description) | ||
| assert.Equal(t, tt.expectedQueue, conf.Core.QueueNum, | ||
| "QueueNum: %s", tt.description) | ||
|
|
||
| // Ensure values are always positive (safety check) | ||
| assert.True(t, conf.Core.WorkerNum > 0, | ||
| "WorkerNum should be positive to prevent panics") | ||
| assert.True(t, conf.Core.QueueNum > 0, | ||
| "QueueNum should be positive to prevent panics") | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Tests re-implement prod logic; call the real loader instead
These assertions duplicate the fallback rules inline, so they can pass even if the production logic regresses. Drive the config via YAML + LoadConf to exercise the real defaults/sanitizers.
Apply:
func TestNegativeValues(t *testing.T) {
- tests := []struct {
- name string
- workerNum int64
- queueNum int64
- expectedWorker int64
- expectedQueue int64
- description string
- }{
- {
- name: "negative_worker_num_should_use_cpu_count",
- workerNum: -5,
- queueNum: DefaultQueueNum,
- expectedWorker: int64(runtime.NumCPU()),
- expectedQueue: DefaultQueueNum,
- description: "Negative WorkerNum should be replaced with CPU count",
- },
- {
- name: "negative_queue_num_should_use_default",
- workerNum: 4, // Valid positive value
- queueNum: -100,
- expectedWorker: 4,
- expectedQueue: DefaultQueueNum,
- description: "Negative QueueNum should be replaced with default",
- },
- {
- name: "both_negative_should_use_defaults",
- workerNum: -1,
- queueNum: -1,
- expectedWorker: int64(runtime.NumCPU()),
- expectedQueue: DefaultQueueNum,
- description: "Both negative values should be replaced with defaults",
- },
- {
- name: "zero_worker_num_should_use_cpu_count",
- workerNum: 0,
- queueNum: DefaultQueueNum,
- expectedWorker: int64(runtime.NumCPU()),
- expectedQueue: DefaultQueueNum,
- description: "Zero WorkerNum should be replaced with CPU count",
- },
- {
- name: "zero_queue_num_should_use_default",
- workerNum: 4,
- queueNum: 0,
- expectedWorker: 4,
- expectedQueue: DefaultQueueNum,
- description: "Zero QueueNum should be replaced with default",
- },
- }
-
- for _, tt := range tests {
- t.Run(tt.name, func(t *testing.T) {
- // Create config with test values
- conf := &ConfYaml{}
- conf.Core.WorkerNum = tt.workerNum
- conf.Core.QueueNum = tt.queueNum
-
- // Apply the same logic as loadConfigFromViper
- if conf.Core.WorkerNum <= 0 {
- conf.Core.WorkerNum = int64(runtime.NumCPU())
- }
- if conf.Core.QueueNum <= 0 {
- conf.Core.QueueNum = DefaultQueueNum
- }
-
- // Verify results
- assert.Equal(t, tt.expectedWorker, conf.Core.WorkerNum,
- "WorkerNum: %s", tt.description)
- assert.Equal(t, tt.expectedQueue, conf.Core.QueueNum,
- "QueueNum: %s", tt.description)
-
- // Ensure values are always positive (safety check)
- assert.True(t, conf.Core.WorkerNum > 0,
- "WorkerNum should be positive to prevent panics")
- assert.True(t, conf.Core.QueueNum > 0,
- "QueueNum should be positive to prevent panics")
- })
- }
+ tests := []struct {
+ name string
+ workerNum int64
+ queueNum int64
+ expectedWorker int64
+ expectedQueue int64
+ }{
+ {"negative_worker_num_should_use_cpu_count", -5, DefaultQueueNum, int64(runtime.NumCPU()), DefaultQueueNum},
+ {"negative_queue_num_should_use_default", 4, -100, 4, DefaultQueueNum},
+ {"both_negative_should_use_defaults", -1, -1, int64(runtime.NumCPU()), DefaultQueueNum},
+ {"zero_worker_num_should_use_cpu_count", 0, DefaultQueueNum, int64(runtime.NumCPU()), DefaultQueueNum},
+ {"zero_queue_num_should_use_default", 4, 0, 4, DefaultQueueNum},
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ dir := t.TempDir()
+ path := filepath.Join(dir, "cfg.yml")
+ yml := fmt.Sprintf("core:\n worker_num: %d\n queue_num: %d\n", tt.workerNum, tt.queueNum)
+ err := os.WriteFile(path, []byte(yml), 0o600)
+ assert.NoError(t, err)
+
+ conf, err := LoadConf(path)
+ assert.NoError(t, err)
+ assert.Equal(t, tt.expectedWorker, conf.Core.WorkerNum)
+ assert.Equal(t, tt.expectedQueue, conf.Core.QueueNum)
+ assert.Greater(t, conf.Core.WorkerNum, int64(0))
+ assert.Greater(t, conf.Core.QueueNum, int64(0))
+ })
+ }
}Add imports:
import (
// ...
"fmt"
"path/filepath"
)1 participant
Summary by CodeRabbit