Skip to content

Conversation

@mangelajo
Copy link
Member

@mangelajo mangelajo commented Oct 28, 2025

Summary by CodeRabbit

  • Tests
    • Improved lease controller test timing to ensure deterministic test execution and prevent race conditions.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 28, 2025

Walkthrough

The change staggered BeginTime assignments for two leases in the test suite by introducing distinct future timestamps—1 second for lease1 and 1.1 seconds for lease2—to ensure deterministic ordering and prevent timing races when leases are concurrently created and verified.

Changes

Cohort / File(s) Summary
Test timing determinism
internal/controller/lease_controller_test.go
Modified lease creation timestamps from shared BeginTime to distinct, staggered values (1s and 1.1s offsets) to eliminate race conditions and ensure predictable ordering in concurrent lease scenarios

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Isolated test-only change with no production code impact
  • Simple temporal offset adjustment with no control flow modifications
  • No logic changes or error handling alterations

Possibly related PRs

Poem

🐰 Two leases ticking, now perfectly spaced,
A hundred milliseconds in time-based grace,
No races to worry, no flaky surprise,
Just deterministic tests that always pass—oh my! 🕐

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "fix race condition in beginlease test" directly and accurately reflects the main change in the pull request. The changeset modifies a test in lease_controller_test.go to fix a race condition by assigning distinct begin times to two leases instead of using a single shared value, ensuring deterministic ordering. The title is concise, specific, and meaningful—clearly communicating that this PR addresses a race condition in a test without vague terminology or unnecessary noise. A developer scanning the commit history would immediately understand the purpose of this change.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-test-race

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 37c64f8 and 14dda17.

📒 Files selected for processing (1)
  • internal/controller/lease_controller_test.go (1 hunks)
⏰ 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). (7)
  • GitHub Check: lint-go
  • GitHub Check: deploy-with-operator
  • GitHub Check: deploy-kind
  • GitHub Check: e2e-tests (ubuntu-24.04-arm)
  • GitHub Check: tests
  • GitHub Check: e2e-tests (ubuntu-24.04)
  • GitHub Check: e2e-tests-release-0-7
🔇 Additional comments (1)
internal/controller/lease_controller_test.go (1)

1663-1678: LGTM! Excellent fix for the race condition.

The 100ms stagger between lease1BeginTime and lease2BeginTime ensures deterministic ordering when two leases compete for the same exporter. This prevents timing-dependent test failures while maintaining the test's original intent to verify sequential lease acquisition.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.


// Use same BeginTime for both to avoid timing races
sharedBeginTime := metav1.NewTime(time.Now().Truncate(time.Second).Add(1 * time.Second))
// Give lease1 an earlier BeginTime to ensure deterministic ordering
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the controller is non deterministic when two leases share begin time.

@mangelajo mangelajo merged commit 7d24dd6 into main Oct 29, 2025
9 checks passed
@mangelajo mangelajo deleted the fix-test-race branch October 29, 2025 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants