fix: update test config to use smaller time window#330
Conversation
WalkthroughConfiguration thresholds for health event rules are reduced (MultipleRemediations from 180s to 90s, RepeatedXidError from 180s to 120s), with corresponding test timing adjustments to validate the new durations. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
|
/ok to test 11f4a82 |
tanishagoyal2
force-pushed
the
tests_fix
branch
from
11f4a82 to
c37d5b0
Compare
tanishagoyal2
changed the title
tanishagoyal2
force-pushed
the
tests_fix
branch
from
bacfac1 to
eaed128
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
tests/health_events_analyzer_test.go (1)
124-133: Add a 120-second wait in TestRepeatedXIDRule Setup, consistent with TestMultipleRemediationsCompleted.The RepeatedXidError rule has a 120-second observation window (configured as "2 minutes"), but unlike TestMultipleRemediationsCompleted—which explicitly waits 90 seconds in its Setup phase—TestRepeatedXIDRule does not wait for this window to age out before starting its test scenario.
Since both tests run sequentially on the same cluster and namespace, and TeardownHealthEventsAnalyzer does not explicitly wait for rule observation windows to expire, events from prior test runs could remain active in the RepeatedXidError analyzer's internal state and affect burst window tracking.
For consistency and test isolation:
feature.Setup(func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context { _, testCtx = helpers.SetupHealthEventsAnalyzerTest(ctx, t, c, "data/health-events-analyzer-config.yaml", "health-events-analyzer-test") t.Log("Waiting 120 seconds for the RepeatedXidError rule time window to complete") time.Sleep(120 * time.Second) return ctx })This ensures a clean slate for burst window logic, matching the pattern established in TestMultipleRemediationsCompleted.
🧹 Nitpick comments (1)
tests/health_events_analyzer_test.go (1)
40-41: Extract the hardcoded time window to a constant.The 90-second value is hardcoded here and tightly coupled to the configuration threshold. If the config value changes, this test will need manual updates.
Consider extracting this to a named constant at the package level:
+const multipleRemediationsTimeWindow = 90 * time.Second + func TestMultipleRemediationsCompleted(t *testing.T) { feature := features.New("TestMultipleRemediationsCompleted"). WithLabel("suite", "health-event-analyzer")Then use it in the test:
- t.Log("Waiting 90 seconds for the MultipleRemediations rule time window to complete") - time.Sleep(90 * time.Second) + t.Logf("Waiting %v for the MultipleRemediations rule time window to complete", multipleRemediationsTimeWindow) + time.Sleep(multipleRemediationsTimeWindow)This improves maintainability and makes the intent clearer. Ideally, this value could be read from the test configuration file itself to eliminate duplication entirely.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/data/health-events-analyzer-config.yaml(4 hunks)tests/health_events_analyzer_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/data/health-events-analyzer-config.yaml
⏰ 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). (30)
- GitHub Check: container-build-test (gpu-health-monitor-dcgm4, make -C health-monitors/gpu-health-monitor docker...
- GitHub Check: ko-build-test (platform-connectors, .)
- GitHub Check: ko-build-test (health-monitors/csp-health-monitor, ./cmd/maintenance-notifier)
- GitHub Check: ko-build-test (health-events-analyzer, .)
- GitHub Check: container-build-test (log-collector, make -C log-collector docker-build-log-collector)
- GitHub Check: ko-build-test (fault-quarantine, .)
- GitHub Check: ko-build-test (labeler, .)
- GitHub Check: container-build-test (syslog-health-monitor, make -C health-monitors/syslog-health-monitor docker...
- GitHub Check: ko-build-test (fault-remediation, .)
- GitHub Check: ko-build-test (health-monitors/csp-health-monitor, ./cmd/csp-health-monitor)
- GitHub Check: ko-build-test (node-drainer, .)
- GitHub Check: container-build-test (gpu-health-monitor-dcgm3, make -C health-monitors/gpu-health-monitor docker...
- GitHub Check: container-build-test (file-server-cleanup, make -C log-collector docker-build-file-server-cleanup)
- GitHub Check: modules-lint-test (node-drainer)
- GitHub Check: modules-lint-test (tests)
- GitHub Check: modules-lint-test (fault-remediation)
- GitHub Check: modules-lint-test (janitor)
- GitHub Check: modules-lint-test (fault-quarantine)
- GitHub Check: modules-lint-test (labeler)
- GitHub Check: modules-lint-test (health-events-analyzer)
- GitHub Check: modules-lint-test (store-client)
- GitHub Check: modules-lint-test (platform-connectors)
- GitHub Check: modules-lint-test (commons)
- GitHub Check: modules-lint-test (data-models)
- GitHub Check: health-monitors-lint-test (csp-health-monitor)
- GitHub Check: health-monitors-lint-test (gpu-health-monitor, true, true, false)
- GitHub Check: health-monitors-lint-test (syslog-health-monitor)
- GitHub Check: CodeQL PR Analysis
- GitHub Check: E2E Tests (AMD64)
- GitHub Check: E2E Tests (ARM64)
|
This pull request has been automatically locked since it has been closed for 90 days with no further activity. Please open a new pull request for related changes. |
2 participants
Summary
Type of Change
Component(s) Affected
Testing
Checklist
Summary by CodeRabbit