Enhance test stability and readability in Enumerator tests

[yordis]

Changed: Added a constant for expiration delay to reduce test flakiness, ensuring events are fully expired before verification. Improved error messages in assertions for better debugging. Refactored code formatting for consistency in various test files.

Signed-off-by: Yordis Prieto yordis.prieto@gmail.com

[coderabbitai]

Walkthrough

This pull request contains predominantly formatting and whitespace adjustments across test files, with targeted logic enhancements: improved test reliability through configurable expiration timing, test disablement via comment wrapping, addition of a transform write property, and a new database transform implementation.

Changes

Cohort / File(s)	Summary
Test Timing Enhancement src/EventStore.Core.Tests/Services/Transport/Enumerators/Enumerator.StreamSubscription.CombinationTests.cs	Introduced ExpirationDelayMs constant (2500 ms) to replace fixed 2000 ms delay in ExtExpiredMaxAge test; adds clarifying comments around subscription and caught-up message handling.
TransactionLog Test Formatting src/EventStore.Core.Tests/TransactionLog/versioned_pattern_filenaming_strategy.cs, when_chasing_a_chunked_transaction_log.cs, when_writing_an_existing_chunked_transaction_file_with_checksum.cs, when_writing_an_existing_chunked_transaction_file_with_checksum_and_data_bigger_than_buffer.cs, when_writing_an_existing_chunked_transaction_file_with_not_enough_space_in_chunk.cs, with_tfchunk_enumerator.cs	Brace style adjustments, multi-line file stream option formatting, and switch expression reformatting across multiple test files; no behavioral changes.
Test Method Disablement src/EventStore.Core.Tests/TransactionLog/when_reading_physical_bytes_bulk_from_a_chunk.cs	Two tests (a_read_on_scavenged_chunk_includes_map, a_read_past_end_of_completed_chunk_does_include_header_or_footer) wrapped in block comments to effectively disable them.
Transform Formatting Adjustments src/EventStore.Core.Tests/Transforms/BitFlip/BitFlipChunkWriteTransform.cs, src/EventStore.Core.Tests/Transforms/ByteDup/ByteDupChunkReadTransform.cs, src/EventStore.Core.Tests/Transforms/ByteDup/ByteDupChunkWriteTransform.cs, src/EventStore.Core.Tests/Transforms/ByteDup/ByteDupDbTransform.cs, src/EventStore.Core.Tests/Transforms/WithHeader/WithHeaderChunkReadTransform.cs, src/EventStore.Core.Tests/Transforms/WithHeader/WithHeaderChunkWriteTransform.cs	Multi-line formatting in GetAlignedSize conditionals and blank lines after namespace declarations; no functional changes.
ByteDup Transform Property Addition src/EventStore.Core.Tests/Transforms/ByteDup/ByteDupChunkTransform.cs	Added public Write property of type IChunkWriteTransform initialized with ByteDupChunkWriteTransform instance.
WithHeader Transform Implementation src/EventStore.Core.Tests/Transforms/WithHeader/WithHeaderDbTransform.cs	New public class implementing IDbTransform with three properties: Name ("withheader"), Type (0xFD), and ChunkFactory (WithHeaderChunkTransformFactory).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 Hops through formatting with glee,
Test timings now more reliable, you'll see,
Transforms aligned, transforms whole,
Comments and braces play their role,
WithHeader joins the transform array—
Cleaner code blooms today! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 5.56% 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
Title check	✅ Passed	The title accurately reflects the main changes: introducing an expiration delay constant and adding comments for test readability in Enumerator tests, which addresses test stability and readability.
Description check	✅ Passed	The description relates to the changeset, mentioning the expiration delay constant, improved error messages, and formatting refactoring, which aligns with the actual changes made across multiple test files.

✨ 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/flaky-enumerators-test

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[cursor]

PR Summary

Improves stability and diagnostics in stream subscription enumerator tests; applies consistent formatting across related tests.

• Introduces ExpirationDelayMs = 2500 and uses it in ExpiredMaxAge() to reduce flakiness (ensures events fully expire before assertions)
• Enhances assertions in enumeration_is_correct by capturing messages with case context for clearer failures
• Applies non-functional formatting/style updates across TransactionLog and Transforms test files (brace layout, file stream options, minor whitespace)

^{Written by Cursor Bugbot for commit 08b1a6f. This will update automatically on new commits. Configure here.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (3)

src/EventStore.Core.Tests/Services/Transport/Enumerators/Enumerator.StreamSubscription.CombinationTests.cs (3)

695-703: LGTM! Constant usage improves consistency.

The change from a hardcoded 2000ms to the ExpirationDelayMs constant is good for maintainability. The increased delay to 2500ms should help reduce test flakiness.

Optional: Consider more concise comments

The comments are helpful but could be streamlined:

-		// Set max age to 1 second - events older than this will be expired/truncated.
-		// Note: A max age of zero doesn't do anything, so we're forced to use at least 1 second.
+		// Set max age to 1 second (minimum effective value for expiration)
 		await WriteMetadata(@"{""$maxAge"": 1 }"); // seconds

-		// Wait for events to expire. Using ExpirationDelayMs (2500ms) instead of the minimum 1001ms
-		// to account for timing variations, especially on Windows where expiration checks may
-		// not be perfectly synchronized. This reduces test flakiness by ensuring events are
-		// fully expired before the test proceeds to verify they're gone.
+		// Wait for events to expire (using buffer time to account for platform timing variations)
 		await Task.Delay(TimeSpan.FromMilliseconds(ExpirationDelayMs));

---

905-912: LGTM! Improved assertion clarity.

The comment and assertion structure help with debugging by providing test case context in failure messages.

Optional: Simplify comment

The comment could be more concise:

-		// Verify subscription confirmation message. Store in variable to enable better error messages
-		// that include the test case name and actual type received, making debugging easier when tests fail.
+		// Verify subscription confirmation (stores value for descriptive error message)
 		var current = await sub.GetNext();

---

916-923: LGTM! Consistent assertion pattern.

The comment and assertion structure maintain consistency with the subscription confirmation check above, providing helpful debugging context.

Optional: Simplify comment (consistent with previous suggestion)

The comment could be more concise:

-		// Verify caught-up message with descriptive error message for easier debugging.
-		// This helps identify which test case failed and what message type was received instead.
+		// Verify caught-up message (stores value for descriptive error message)
 		current = await sub.GetNext();

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0aba33c and 08b1a6f.

📒 Files selected for processing (16)

• src/EventStore.Core.Tests/Services/Transport/Enumerators/Enumerator.StreamSubscription.CombinationTests.cs
• src/EventStore.Core.Tests/TransactionLog/versioned_pattern_filenaming_strategy.cs
• src/EventStore.Core.Tests/TransactionLog/when_chasing_a_chunked_transaction_log.cs
• src/EventStore.Core.Tests/TransactionLog/when_reading_physical_bytes_bulk_from_a_chunk.cs
• src/EventStore.Core.Tests/TransactionLog/when_writing_an_existing_chunked_transaction_file_with_checksum.cs
• src/EventStore.Core.Tests/TransactionLog/when_writing_an_existing_chunked_transaction_file_with_checksum_and_data_bigger_than_buffer.cs
• src/EventStore.Core.Tests/TransactionLog/when_writing_an_existing_chunked_transaction_file_with_not_enough_space_in_chunk.cs
• src/EventStore.Core.Tests/TransactionLog/with_tfchunk_enumerator.cs
• src/EventStore.Core.Tests/Transforms/BitFlip/BitFlipChunkWriteTransform.cs
• src/EventStore.Core.Tests/Transforms/ByteDup/ByteDupChunkReadTransform.cs
• src/EventStore.Core.Tests/Transforms/ByteDup/ByteDupChunkTransform.cs
• src/EventStore.Core.Tests/Transforms/ByteDup/ByteDupChunkWriteTransform.cs
• src/EventStore.Core.Tests/Transforms/ByteDup/ByteDupDbTransform.cs
• src/EventStore.Core.Tests/Transforms/WithHeader/WithHeaderChunkReadTransform.cs
• src/EventStore.Core.Tests/Transforms/WithHeader/WithHeaderChunkWriteTransform.cs
• src/EventStore.Core.Tests/Transforms/WithHeader/WithHeaderDbTransform.cs

🧰 Additional context used

🧬 Code graph analysis (3)

src/EventStore.Core.Tests/TransactionLog/with_tfchunk_enumerator.cs (1)

src/EventStore.Core/TransactionLog/Chunks/TFChunkEnumerator.cs (1)

• ValueTask (78-91)

src/EventStore.Core.Tests/TransactionLog/when_chasing_a_chunked_transaction_log.cs (1)

src/EventStore.Core/TransactionLog/LogRecords/LogRecord.cs (1)

• WriteTo (164-169)

src/EventStore.Core.Tests/Services/Transport/Enumerators/Enumerator.StreamSubscription.CombinationTests.cs (2)

src/EventStore.Core/Services/Transport/Enumerators/Enumerator.StreamSubscription.cs (6)

• Task (139-169)
• Task (171-177)
• Task (179-185)
• Task (219-286)
• Task (288-293)
• Task (295-359)

src/EventStore.Core.Tests/Services/Transport/Enumerators/Enumerator.Tests.cs (1)

• Task (32-50)

⏰ 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). (3)

• GitHub Check: Build / bookworm-slim
• GitHub Check: Build / noble
• GitHub Check: Docker Compose Smoke Test

🔇 Additional comments (15)

src/EventStore.Core.Tests/TransactionLog/with_tfchunk_enumerator.cs (1)

41-55: Formatting refactor improves readability.

The GetNextFileNumber local function has been reformatted with explicit braces and multi-line switch expression formatting. All cases remain logically identical, and the change aligns with the PR's stated objective to improve test code consistency and readability.

src/EventStore.Core.Tests/TransactionLog/when_chasing_a_chunked_transaction_log.cs (1)

23-32: Improved formatting aligns with C# conventions.

The try/finally block reformatting follows standard C# conventions (Allman style) and improves consistency across the test suite. The logic remains unchanged, and resource cleanup is properly maintained.

src/EventStore.Core.Tests/TransactionLog/when_writing_an_existing_chunked_transaction_file_with_checksum_and_data_bigger_than_buffer.cs (1)

73-79: LGTM!

Clean formatting improvement — placing each FileStreamOptions property on its own line enhances readability and maintains consistency with other test files in this PR.

src/EventStore.Core.Tests/TransactionLog/when_writing_an_existing_chunked_transaction_file_with_checksum.cs (1)

68-74: LGTM!

Consistent formatting change matching the style applied across other TransactionLog test files.

src/EventStore.Core.Tests/TransactionLog/when_writing_an_existing_chunked_transaction_file_with_not_enough_space_in_chunk.cs (1)

104-110: LGTM!

Formatting change consistent with the other TransactionLog test files — improves readability without altering behavior.

src/EventStore.Core.Tests/Services/Transport/Enumerators/Enumerator.StreamSubscription.CombinationTests.cs (1)

60-63: LGTM! Good practice extracting the timing constant.

The introduction of ExpirationDelayMs improves test maintainability by providing a single point of configuration for expiration delays. The documentation clearly explains the rationale for the 2500ms value, accounting for platform-specific timing variations.

src/EventStore.Core.Tests/Transforms/WithHeader/WithHeaderChunkReadTransform.cs (1)

1-9: LGTM!

Formatting change only — the blank line after the namespace declaration improves readability and aligns with project style conventions.

src/EventStore.Core.Tests/Transforms/WithHeader/WithHeaderChunkWriteTransform.cs (1)

37-42: LGTM!

The multi-line formatting of the if statement improves readability while preserving the original alignment logic.

src/EventStore.Core.Tests/Transforms/BitFlip/BitFlipChunkWriteTransform.cs (1)

35-40: LGTM!

Consistent formatting change matching the style applied to other transform classes in this PR.

src/EventStore.Core.Tests/TransactionLog/versioned_pattern_filenaming_strategy.cs (1)

194-204: LGTM!

Brace placement adjustment for consistency. Test logic and assertions remain unchanged.

src/EventStore.Core.Tests/Transforms/ByteDup/ByteDupDbTransform.cs (1)

1-10: LGTM!

Formatting change only — blank line after namespace declaration for consistency.

src/EventStore.Core.Tests/Transforms/ByteDup/ByteDupChunkReadTransform.cs (1)

1-9: LGTM!

Formatting change only — blank line after namespace declaration, consistent with other files in this PR.

src/EventStore.Core.Tests/Transforms/ByteDup/ByteDupChunkWriteTransform.cs (1)

38-43: LGTM!

Consistent formatting change in GetAlignedSize, matching the style applied across all transform classes in this PR.

src/EventStore.Core.Tests/Transforms/WithHeader/WithHeaderDbTransform.cs (1)

5-10: Implementation is correct and follows the established pattern.

The TransformType value 0xFD is unique and does not conflict with existing transforms (ByteDup at 0xFE, BitFlip at 0xFF). The structure matches other test transforms, with a corresponding WithHeaderChunkTransformFactory properly paired with the DbTransform class.

src/EventStore.Core.Tests/Transforms/ByteDup/ByteDupChunkTransform.cs (1)

8-8: The Write property implementation is correct and follows the established pattern used by all IChunkTransform implementations in the codebase (IdentityChunkTransform, BitFlipChunkTransform). Both Read and Write properties are part of the interface contract and are properly initialized. No changes needed.