Skip to content

Conversation

@Joibel
Copy link
Member

@Joibel Joibel commented Dec 12, 2025

Whilst reviewing the failing test in #15146 I wondered why this test appeared asymmetrical. So I made it symmetrical.

It now tests semaphores and mutexes both before and after the call to CheckWorkflowExistence()

Summary by CodeRabbit

  • Tests
    • Strengthened test assertions for workflow synchronization state management, improving validation of holder and pending entry transitions during workflow existence checks.

✏️ Tip: You can customize this high-level summary in your review settings.

@Joibel
Copy link
Member Author

Joibel commented Dec 12, 2025

@coderabbitai full review

@coderabbitai
Copy link

coderabbitai bot commented Dec 12, 2025

✅ Actions performed

Full review triggered.

@Joibel Joibel marked this pull request as draft December 12, 2025 11:25
@coderabbitai
Copy link

coderabbitai bot commented Dec 12, 2025

📝 Walkthrough

Walkthrough

This pull request adds pre-state and post-state assertions to TestCheckWorkflowExistence to verify the state of mutex and semaphore holders and pending lists before and after workflow existence checks, validating expected state transitions.

Changes

Cohort / File(s) Change Summary
Test state assertions for sync manager
workflow/sync/sync_manager_test.go
Added pre-state assertions verifying current holders and pending lists before CheckWorkflowExistence call, and post-state assertions validating that mutex current holders are emptied while pending entries remain, and semaphore holders and pendings are cleared.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

  • Test-only modifications with straightforward assertion additions
  • No production code changes or logic modifications
  • Single file affected with consistent pattern of state verification assertions

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description check ⚠️ Warning The description lacks required sections from the template, particularly the 'Fixes #' reference, motivation details, and verification information expected for PR submissions. Add the issue reference (Fixes #15146), expand motivation and verification sections, and follow the conventional commits template structure more closely.
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 (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: adding checks for both mutex and semaphore in the test, which aligns with the implementation changes.
✨ 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 sync-test-improvement

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6648694 and 82be07b.

📒 Files selected for processing (1)
  • workflow/sync/sync_manager_test.go (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
workflow/sync/sync_manager_test.go (1)
test/e2e/fixtures/given.go (1)
  • NoError (244-247)
⏰ 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
  • GitHub Check: Windows Unit Tests
  • GitHub Check: Build Binaries (cli)
  • GitHub Check: Unit Tests
  • GitHub Check: Build Binaries (controller)
  • GitHub Check: argo-images (argocli)
  • GitHub Check: argo-images (argoexec)
🔇 Additional comments (1)
workflow/sync/sync_manager_test.go (1)

1057-1089: LGTM! Excellent test improvement.

The addition of symmetric pre-state and post-state assertions for both mutex and semaphore makes this test more comprehensive and maintainable. The clear comments explaining each assertion block help document the expected behavior of CheckWorkflowExistence().

The test now properly verifies:

  • Initial state is set up correctly (both mutex and semaphore have 1 holder and 1 pending)
  • Cleanup removes non-existent workflow holders while preserving valid pending entries
  • Differential behavior between mutex (preserves pending "test1") and semaphore (removes pending "test2")

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

@Joibel Joibel marked this pull request as ready for review December 12, 2025 13:27
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.

1 participant