feat(e2e): test suite with rate limiter scenarios and file flag #82
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe PR tightens e2e tests by removing debug logs, replacing ad-hoc error checks with testify/require assertions, and adding file-based publish test cases. It introduces e2e/ratelimit_scenarios_test.go with multiple rate-limit and usage-tracking scenarios (CLI, gRPC, file). ratelimit_test.go now parses and compares usage publish counts before/after publishes. validators.go was enhanced to extract richer usage metrics (hour/sec limits, bytes used) and exposes new public fields. Test startup no longer skips when MUMP2P_E2E_SKIP is set. Sequence Diagram(s)(Skipped) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
📜 Recent review detailsConfiguration used: Repository: getoptimum/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
e2e/cross_node_test.go (1)
75-76: Reduce reliance on fixedtime.Sleepdelays for readiness and pacingMultiple tests use hard-coded sleeps to wait for subscribers and to “avoid rate limiting”. This can both slow the suite and introduce flakiness if timing characteristics change.
Where feasible, consider more deterministic synchronization (e.g., waiting for a specific log line from the subscriber, CLI readiness signal, or a simple handshake over stdout/stderr) and replacing global pacing sleeps with a tighter, condition-based check. This will keep the e2e tests faster and more stable over time.
Also applies to: 114-115, 142-143, 189-190, 245-246
e2e/ratelimit_test.go (1)
23-60: Make publish-count parsing fail fast instead of silently returning 0Right now
parsePublishCountreturns0on any parse error:func parsePublishCount(countStr string) int { count, err := strconv.Atoi(countStr) if err != nil { return 0 } return count }In this test you only assert
afterCount >= beforeCount, so ifPublishCountever becomes missing/non‑numeric, both values can be0and the test still passes, masking a real regression in the CLI output format. In contrast, the new rate‑limit scenario tests assert againstbeforeCount+N, so they would fail on parse errors — this inconsistency makes the suite harder to reason about.I’d make parse errors explicit test failures by threading
*testing.Tinto the helper and usingrequire.NoError, and update all call sites (here and ine2e/ratelimit_scenarios_test.go) to passt:-usageInfoBefore, err := validator.ValidateUsage() +usageInfoBefore, err := validator.ValidateUsage() require.NoError(t, err, "Failed to parse usage stats") … -// Parse publish counts to verify they increased -beforeCount := parsePublishCount(usageInfoBefore.PublishCount) -afterCount := parsePublishCount(usageInfoAfter.PublishCount) +// Parse publish counts to verify they increased +beforeCount := parsePublishCount(t, usageInfoBefore.PublishCount) +afterCount := parsePublishCount(t, usageInfoAfter.PublishCount)-// parsePublishCount parses the publish count string to an integer -func parsePublishCount(countStr string) int { - count, err := strconv.Atoi(countStr) - if err != nil { - return 0 - } - return count -} +// parsePublishCount parses the publish count string to an integer and fails the test on errors. +func parsePublishCount(t *testing.T, countStr string) int { + t.Helper() + count, err := strconv.Atoi(countStr) + require.NoError(t, err, "Failed to parse PublishCount from usage output: %q", countStr) + return count +}And similarly adjust all usages in
e2e/ratelimit_scenarios_test.go:-beforeCount := parsePublishCount(usageInfoBefore.PublishCount) +beforeCount := parsePublishCount(t, usageInfoBefore.PublishCount) -afterCount := parsePublishCount(usageInfoAfter.PublishCount) +afterCount := parsePublishCount(t, usageInfoAfter.PublishCount)Also at:
e2e/ratelimit_scenarios_test.go:29,65,91,123,153,184.This keeps the tests strict and ensures any change in the CLI’s usage output format breaks quickly and loudly instead of being papered over by a default
0.Also applies to: 62-68
📜 Review details
Configuration used: Repository: getoptimum/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
e2e/cross_node_test.go(2 hunks)e2e/integration_test.go(0 hunks)e2e/publish_test.go(3 hunks)e2e/ratelimit_scenarios_test.go(1 hunks)e2e/ratelimit_test.go(3 hunks)e2e/subscribe_test.go(0 hunks)
💤 Files with no reviewable changes (2)
- e2e/integration_test.go
- e2e/subscribe_test.go
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (Custom checks)
**/*.go: Watch for goroutine leaks, unbounded channels, and racy access; ensure contexts are plumbed and respected
If code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Files:
e2e/publish_test.goe2e/ratelimit_scenarios_test.goe2e/ratelimit_test.goe2e/cross_node_test.go
⚙️ CodeRabbit configuration file
Review Go code for: idiomatic patterns, error handling (check all errors), concurrency safety (context propagation, goroutine cleanup), test coverage for changed logic, exported API stability, memory efficiency (avoid unnecessary allocations), and prefer standard library over third-party when reasonable.
Files:
e2e/publish_test.goe2e/ratelimit_scenarios_test.goe2e/ratelimit_test.goe2e/cross_node_test.go
**/*.{go,rs,js,ts,py,java,cpp,c,rb,php}
📄 CodeRabbit inference engine (Custom checks)
Flag security risks: SQL injection, command injection, path traversal, unvalidated inputs, hardcoded credentials, insecure crypto usage (MD5/SHA1), and missing rate limiting on public endpoints
Files:
e2e/publish_test.goe2e/ratelimit_scenarios_test.goe2e/ratelimit_test.goe2e/cross_node_test.go
**/*
⚙️ CodeRabbit configuration file
**/*: When you find the same underlying issue in multiple locations (same pattern, same fix):
- Do NOT post separate comments for each occurrence.
- Post a single primary comment on the first occurrence.
- In that comment, include a short list of “Also at: file:line” references
for the other locations (e.g. 'Also at: foo.go:42, bar.go:17').
Prioritize signal over volume: one strong comment that references multiple
locations is preferred over many near-identical comments.
Files:
e2e/publish_test.goe2e/ratelimit_scenarios_test.goe2e/ratelimit_test.goe2e/cross_node_test.go
**/*_test.go
⚙️ CodeRabbit configuration file
Verify tests are deterministic and isolated (no shared global state). Prefer table-driven tests and subtests with clear names (TestX_Scenario_ExpectedBehavior). Use t.Parallel() only when state is truly isolated; ensure data races won't occur under -race. Avoid time.Sleep—prefer synchronization via channels, contexts with deadlines, WaitGroups, and condition variables. Use t.Cleanup() (or defer + t.Cleanup) for resource cleanup; use t.TempDir() for filesystem. Seed randomness deterministically (rand.New(rand.NewSource(…))). Prefer httptest/httptest.Server over real network calls; avoid external services. Make assertions descriptive with context. Verify goroutine lifecycle (no leaks) and ensure all timers/tickers are stopped. Avoid flaky patterns (wall-clock timing, sleeps, unordered map iteration assumptions). Utilize "require" package.
Files:
e2e/publish_test.goe2e/ratelimit_scenarios_test.goe2e/ratelimit_test.goe2e/cross_node_test.go
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: getoptimum/optimum-infra PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-12-02T17:16:21.126Z
Learning: Applies to **/*_test.go : If code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Learnt from: CR
Repo: getoptimum/optimum-gateway PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-11-27T11:40:12.459Z
Learning: Applies to **/*_test.go : If code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Learnt from: CR
Repo: getoptimum/optimum-common PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-12-02T11:56:13.180Z
Learning: Applies to **/*_test.go : If code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Learnt from: CR
Repo: getoptimum/optimum-bootstrap PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-11-28T11:27:28.785Z
Learning: Applies to **/*.go : If code changes affect Go packages, verify that tests cover changed logic and flag risky changes lacking tests
Learnt from: CR
Repo: getoptimum/docs PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-11-28T04:42:48.134Z
Learning: Applies to **/*.go : If code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Learnt from: CR
Repo: getoptimum/optimumroute-rs PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-12-02T17:05:20.122Z
Learning: Applies to **/*.go : If code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Learnt from: CR
Repo: getoptimum/optimum-dev-setup-guide PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-11-27T10:53:12.793Z
Learning: Applies to **/*.go : If code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Learnt from: CR
Repo: getoptimum/mump2p-cli PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-11-28T19:55:52.117Z
Learning: Applies to **/*.go : If code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Learnt from: CR
Repo: getoptimum/optimum-proxy PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-12-02T11:58:48.687Z
Learning: Applies to **/*.go : If Go code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
📚 Learning: 2025-12-02T17:16:21.126Z
Learnt from: CR
Repo: getoptimum/optimum-infra PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-12-02T17:16:21.126Z
Learning: Applies to **/*_test.go : If code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Applied to files:
e2e/publish_test.goe2e/ratelimit_scenarios_test.goe2e/ratelimit_test.goe2e/cross_node_test.go
📚 Learning: 2025-11-28T04:42:48.134Z
Learnt from: CR
Repo: getoptimum/docs PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-11-28T04:42:48.134Z
Learning: Applies to **/*.go : If code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Applied to files:
e2e/publish_test.goe2e/ratelimit_scenarios_test.goe2e/cross_node_test.go
📚 Learning: 2025-11-28T11:27:28.785Z
Learnt from: CR
Repo: getoptimum/optimum-bootstrap PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-11-28T11:27:28.785Z
Learning: Applies to **/*.go : If code changes affect Go packages, verify that tests cover changed logic and flag risky changes lacking tests
Applied to files:
e2e/publish_test.goe2e/ratelimit_scenarios_test.goe2e/cross_node_test.go
📚 Learning: 2025-12-02T11:58:48.687Z
Learnt from: CR
Repo: getoptimum/optimum-proxy PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-12-02T11:58:48.687Z
Learning: Applies to **/*.go : If Go code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Applied to files:
e2e/publish_test.goe2e/ratelimit_scenarios_test.goe2e/cross_node_test.go
🧬 Code graph analysis (3)
e2e/publish_test.go (2)
e2e/cli_runner.go (1)
RunCommand(10-24)e2e/validators.go (1)
NewValidator(16-18)
e2e/ratelimit_scenarios_test.go (3)
e2e/config.go (1)
GetDefaultProxy(11-13)e2e/cli_runner.go (1)
RunCommand(10-24)e2e/validators.go (1)
NewValidator(16-18)
e2e/ratelimit_test.go (1)
e2e/validators.go (1)
NewValidator(16-18)
🔇 Additional comments (1)
e2e/cross_node_test.go (1)
226-230: Health status assertion is clear and appropriateAsserting
healthInfo.Status == "ok"after a successful health check call makes the test’s expectation explicit and ensures we don’t silently accept degraded states. This is a good tightening of the health check behavior.
Walentalien
left a comment
Walentalien
left a comment
There was a problem hiding this comment.
The tests in rate_limit scenarios are not testing ratelimiting but cheching usage is increasing, add tests that exceed limit
Those are not run in parallel so can't you changes the expected_amount >= initial+delta to expected_amount==initial + delta?
Also why do we need this in testmain:
if os.Getenv("MUMP2P_E2E_SKIP") == "1" {
fmt.Fprintln(os.Stderr, "[e2e] skipping CLI smoke tests (MUMP2P_E2E_SKIP=1)")
os.Exit(0)
}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Repository: getoptimum/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
e2e/publish_test.go(3 hunks)e2e/ratelimit_scenarios_test.go(1 hunks)e2e/ratelimit_test.go(3 hunks)e2e/suite_test.go(0 hunks)e2e/validators.go(2 hunks)
💤 Files with no reviewable changes (1)
- e2e/suite_test.go
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (Custom checks)
**/*.go: Watch for goroutine leaks, unbounded channels, and racy access; ensure contexts are plumbed and respected
If code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Files:
e2e/publish_test.goe2e/validators.goe2e/ratelimit_test.goe2e/ratelimit_scenarios_test.go
⚙️ CodeRabbit configuration file
Review Go code for: idiomatic patterns, error handling (check all errors), concurrency safety (context propagation, goroutine cleanup), test coverage for changed logic, exported API stability, memory efficiency (avoid unnecessary allocations), and prefer standard library over third-party when reasonable.
Files:
e2e/publish_test.goe2e/validators.goe2e/ratelimit_test.goe2e/ratelimit_scenarios_test.go
**/*.{go,rs,js,ts,py,java,cpp,c,rb,php}
📄 CodeRabbit inference engine (Custom checks)
Flag security risks: SQL injection, command injection, path traversal, unvalidated inputs, hardcoded credentials, insecure crypto usage (MD5/SHA1), and missing rate limiting on public endpoints
Files:
e2e/publish_test.goe2e/validators.goe2e/ratelimit_test.goe2e/ratelimit_scenarios_test.go
**/*
⚙️ CodeRabbit configuration file
**/*: When you find the same underlying issue in multiple locations (same pattern, same fix):
- Do NOT post separate comments for each occurrence.
- Post a single primary comment on the first occurrence.
- In that comment, include a short list of “Also at: file:line” references
for the other locations (e.g. 'Also at: foo.go:42, bar.go:17').
Prioritize signal over volume: one strong comment that references multiple
locations is preferred over many near-identical comments.
Files:
e2e/publish_test.goe2e/validators.goe2e/ratelimit_test.goe2e/ratelimit_scenarios_test.go
**/*_test.go
⚙️ CodeRabbit configuration file
Verify tests are deterministic and isolated (no shared global state). Prefer table-driven tests and subtests with clear names (TestX_Scenario_ExpectedBehavior). Use t.Parallel() only when state is truly isolated; ensure data races won't occur under -race. Avoid time.Sleep—prefer synchronization via channels, contexts with deadlines, WaitGroups, and condition variables. Use t.Cleanup() (or defer + t.Cleanup) for resource cleanup; use t.TempDir() for filesystem. Seed randomness deterministically (rand.New(rand.NewSource(…))). Prefer httptest/httptest.Server over real network calls; avoid external services. Make assertions descriptive with context. Verify goroutine lifecycle (no leaks) and ensure all timers/tickers are stopped. Avoid flaky patterns (wall-clock timing, sleeps, unordered map iteration assumptions). Utilize "require" package.
Files:
e2e/publish_test.goe2e/ratelimit_test.goe2e/ratelimit_scenarios_test.go
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: getoptimum/optimum-infra PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-12-02T17:16:21.126Z
Learning: Applies to **/*_test.go : If code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Learnt from: CR
Repo: getoptimum/optimum-gateway PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-11-27T11:40:12.459Z
Learning: Applies to **/*_test.go : If code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Learnt from: CR
Repo: getoptimum/optimum-common PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-12-02T11:56:13.180Z
Learning: Applies to **/*_test.go : If code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Learnt from: CR
Repo: getoptimum/optimum-bootstrap PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-11-28T11:27:28.785Z
Learning: Applies to **/*.go : If code changes affect Go packages, verify that tests cover changed logic and flag risky changes lacking tests
Learnt from: CR
Repo: getoptimum/docs PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-11-28T04:42:48.134Z
Learning: Applies to **/*.go : If code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Learnt from: CR
Repo: getoptimum/optimumroute-rs PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-12-02T17:05:20.122Z
Learning: Applies to **/*.go : If code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Learnt from: CR
Repo: getoptimum/optimum-dev-setup-guide PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-11-27T10:53:12.793Z
Learning: Applies to **/*.go : If code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Learnt from: CR
Repo: getoptimum/mump2p-cli PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-11-28T19:55:52.117Z
Learning: Applies to **/*.go : If code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Learnt from: CR
Repo: getoptimum/optimum-proxy PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-12-02T11:58:48.687Z
Learning: Applies to **/*.go : If Go code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
📚 Learning: 2025-12-02T17:16:21.126Z
Learnt from: CR
Repo: getoptimum/optimum-infra PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-12-02T17:16:21.126Z
Learning: Applies to **/*_test.go : If code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Applied to files:
e2e/publish_test.goe2e/ratelimit_scenarios_test.go
📚 Learning: 2025-11-28T04:42:48.134Z
Learnt from: CR
Repo: getoptimum/docs PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-11-28T04:42:48.134Z
Learning: Applies to **/*.go : If code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Applied to files:
e2e/publish_test.goe2e/ratelimit_scenarios_test.go
📚 Learning: 2025-11-28T11:27:28.785Z
Learnt from: CR
Repo: getoptimum/optimum-bootstrap PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-11-28T11:27:28.785Z
Learning: Applies to **/*.go : If code changes affect Go packages, verify that tests cover changed logic and flag risky changes lacking tests
Applied to files:
e2e/publish_test.goe2e/ratelimit_scenarios_test.go
📚 Learning: 2025-12-02T11:58:48.687Z
Learnt from: CR
Repo: getoptimum/optimum-proxy PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-12-02T11:58:48.687Z
Learning: Applies to **/*.go : If Go code changes affect Go packages, verify that tests cover changed logic; flag risky changes lacking tests
Applied to files:
e2e/publish_test.goe2e/ratelimit_scenarios_test.go
📚 Learning: 2025-12-02T11:56:13.180Z
Learnt from: CR
Repo: getoptimum/optimum-common PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2025-12-02T11:56:13.180Z
Learning: Applies to **/*.go : Watch for goroutine leaks, unbounded channels, and racy access; ensure contexts are plumbed and respected
Applied to files:
e2e/ratelimit_scenarios_test.go
🧬 Code graph analysis (2)
e2e/ratelimit_test.go (1)
e2e/validators.go (1)
NewValidator(16-18)
e2e/ratelimit_scenarios_test.go (3)
e2e/cli_runner.go (1)
RunCommand(10-24)e2e/validators.go (1)
NewValidator(16-18)e2e/config.go (1)
GetDefaultProxy(11-13)
🔇 Additional comments (7)
e2e/ratelimit_test.go (1)
23-61: Well-structured before/after usage validation.The pattern of capturing usage before and after the publish, then asserting the exact increment (+1) is clean and deterministic given sequential test execution. The explicit comment about non-parallel execution provides useful context.
e2e/publish_test.go (1)
102-164: Excellent file-based publish test coverage with proper resource management.These tests demonstrate good practices:
t.TempDir()ensures isolated, auto-cleaned temporary directoriesfilepath.Joinfor cross-platform path construction- Both success paths (HTTP, gRPC) and failure paths (file not found, mutually exclusive flags) are covered
- Error messages are validated with
strings.ToLowerfor case-insensitive matchingThis addresses the past review comment about using
t.TempDir()instead of hard-coded/tmppaths.e2e/ratelimit_scenarios_test.go (3)
17-28: Clean helper abstraction with proper test helper marking.The
getInitialPublishCounthelper eliminates repetitive usage fetching across multiple tests. Good use oft.Helper()to ensure error messages point to the actual test caller.
106-114: Smart skip logic prevents flaky or excessively long test runs.The conditional skips when remaining quota is ≤0 or >100 demonstrate good test design:
- Avoids false failures when quota is exhausted
- Prevents extremely long test execution when quota is too high
- Clear skip messages explain why the test didn't run
This makes the test suite more robust across different test environments and timing conditions.
169-185: Proper use oft.TempDir()and robust error message validation.Good practices here:
t.TempDir()for automatic cleanup of the large test file- Flexible error message checking accounts for variations ("message size", "entity too large", "request entity too large") that may come from different layers (CLI rate limiter vs server)
- 10MB size is well above reasonable limits to ensure failure
e2e/validators.go (2)
161-184: Expanded usage parsing with graceful handling of optional fields.The updated
ValidateUsagenow extracts comprehensive metrics:
- Per-hour publish count and limit (required)
- Per-second publish count and limit (optional)
- Data usage and quota (optional)
The logic correctly handles optional fields by checking match length before assigning, allowing the validator to work with varying output formats. This provides the granular data needed for the new rate limiter test scenarios.
207-212: Well-structured UsageInfo with clear field naming.The expanded struct with explicit field names (
PublishLimitPerHour,SecondPublishCount,BytesPublishedMB, etc.) makes usage metrics self-documenting and easy to work with in test assertions.
Walentalien
left a comment
Walentalien
left a comment
There was a problem hiding this comment.
Address my comment and codrabbit, other than that looks ok
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.