-
Notifications
You must be signed in to change notification settings - Fork 266
Fix SSE integration test comparison logic to use correct series pattern #1941
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Both SSE integration tests were incorrectly comparing streaming results to series computed on only the final cached quotes. This fails to account for how streaming indicators process ALL quotes (including pruned ones) and maintain running state. Fixed by: - Computing series on the FULL quote list with all revisions applied - Truncating series results to match final cache size using TakeLast() - Matching the pattern from STC WithCachePruning_MatchesSeriesExactly This ensures tests validate that streaming indicators correctly maintain state even after cache pruning removes early quotes. Co-authored-by: DaveSkender <[email protected]>
Changed from collection expression spread to new List<Quote>(allQuotes) for better performance with large quote lists (2000+ quotes). Co-authored-by: DaveSkender <[email protected]>
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the
📝 WalkthroughWalkthroughThis pull request replaces the public Possibly related issues
Possibly related PRs
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 |
This comment was marked as outdated.
This comment was marked as outdated.
…ering Tests still failing - need to resolve timestamp alignment between streaming results and series results after pruning. The fundamental issue is that TakeLast doesn't account for insert/remove operations changing which quotes are in the cache. Current approach using timestamp filtering is getting warmup nulls included. Need to filter by streaming hub result timestamps, not quote cache timestamps. Co-authored-by: DaveSkender <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Reverted to original test from commit 3f24bf2. Analysis shows: - StcHub test passes with original logic - AllHubs test fails due to EPMA floating point precision issues (not test logic) - Differences are at 15th decimal place, caused by quote revision at index 1600 - Core test logic is sound; need precision tolerance adjustment for EPMA Co-authored-by: DaveSkender <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Implemented the correct comparison pattern following the reference test: - Compute series on FULL quote list - Use TakeLast or timestamp filtering to match cache size - Applied revisions (index 1600 modification) to quote list Current status: - StcHub test: FAILING - Insert operations change cache composition - AllHubs test: Using TakeLast pattern with revisions applied The STC reference test works because it does a simple Add() with no Insert/Remove operations. The SSE tests use Insert() to add earlier quotes back to the cache, which creates a mismatch between TakeLast expectations and actual cache composition. Need guidance on whether to: 1. Modify SSE test to remove Insert operations 2. Use different comparison logic for tests with Insert/Remove Co-authored-by: DaveSkender <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Following user guidance to insert quotes to allQuotesWithRevisions before computing series. However, tests still failing because Insert operations of earlier quotes (80, 100, 500) cause those quotes to be in the cache with warmup null values from their original processing, while series calculation sees them past warmup with values. Current approach: - Built allQuotesWithRevisions including all modifications - Applied revision at index 1600 for AllHubs test - Using TakeLast on quote cache size Issue: Streaming hub "remembers" that quote 80 was processed during warmup (null), but series processes all quotes together so quote 80 is past warmup (has value). This creates a fundamental mismatch that TakeLast doesn't resolve. Need clarification on how to handle Insert operations that add earlier quotes back to cache with their original (null) values. Co-authored-by: DaveSkender <[email protected]>
Signed-off-by: GitHub <[email protected]>
Signed-off-by: GitHub <[email protected]>
…#1957) Signed-off-by: Dave Skender <[email protected]> Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: DaveSkender <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Signed-off-by: GitHub <[email protected]>
Signed-off-by: GitHub <[email protected]>
Signed-off-by: GitHub <[email protected]>
Signed-off-by: GitHub <[email protected]>
Signed-off-by: GitHub <[email protected]>
Signed-off-by: GitHub <[email protected]>
Fix SSE integration test comparison logic
Following the STC reference test pattern and user guidance. All review feedback addressed.
Key Insight:
Insert/Remove operations (operations 1, 2, 3) test rollback mechanisms but DON'T change the final quote data. As the user pointed out: "If you remove a quote then add that same quote back, you obviously shouldn't attempt to add it back again."
Changes Made:
allQuotesWithOperationsin StcHub test: Simple copy ofallQuotes(no modifications)allQuotesWithRevisionsin AllHubs test: Copy ofallQuoteswith ONLY the index 1600 revision appliedTakeLast(cacheSize)pattern from reference testOperations Analysis:
Insert(allQuotes[80])): No-op - quote already in listRemoveAt(100)thenInsert(allQuotes[100])): No-op - remove then re-add same quoteallQuotesWithRevisionsReview Feedback Addressed:
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.