Basic Integration Tests For Live Processing #8
Conversation
LeoPatOZ
changed the title
default
fmt git import
| } | ||
|
|
||
| #[async_trait] | ||
| impl EventCallback for EventCounter { |
There was a problem hiding this comment.
I actually love the callback approach you took with EventCallback - instead of storing the callback itself, you store a callback struct type that implements this callback trait 👍
There was a problem hiding this comment.
yes thought it was simpler glad you like it 😄
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_live_scanning_with_slow_processor() -> anyhow::Result<()> { |
There was a problem hiding this comment.
Just found out that there's a race condition in this test, it fails intermittently...
There was a problem hiding this comment.
The race condition seems to appear when aborting the scanner thread. Because it is slow to process the pending events (events are emitted every ~50ms, while the scanner "runs" every ~100ms), occasionally it doesn't manage to handle the last event before being aborted, causing the assertion below it to fail (in my case it failed because the actual number of processed events was 2, not 3). It could easily happen that the actual number of processed events is just 1.
The solution could be one of the following:
- Add an additional
sleep(200)to allow the scanner thread to "catch up". <- 150ms (the difference in delay) + 50ms buffer just in case; still could theoretically have a race condition in the right circumstances, but much less likely. - Take into account that the last event might not be processed, so update the assertion to state something like
assert!((1..=3).contains(processed.load(Ordering::SeqCst));<- this makes the test an "approximation" of valid behavior, and a poor approximation at that. - Do you have any alternative ideas?
Wdyt?
There was a problem hiding this comment.
I think for now I will add a sleep(200).
Thanks for looking into this - perhaps once we have channels we can expose a method to see if the channels are empty? Im not sure if this is relevant or needed but my reasoning is for any reason a process want to stop the scanner, they must ensure that any callbacks have finished executing.
There was a problem hiding this comment.
We'll try to make the channels testable.
I think we should ensure the scanner does it's job, but I don't think we can make assumptions about what happens when someone intentionally kills the whole process
There was a problem hiding this comment.
I was thinking more of like a pulse check
let bool = scanner.has_pending_callbacks();or like
scanner.safe_abort();There was a problem hiding this comment.
What would the test assertions look like for each?
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_live_scanning_basic() -> anyhow::Result<()> { |
There was a problem hiding this comment.
red alert, just had this test randomly fail, seems it has a race condition too...
My guess is that the scanner_handle.abort() call is again to blame.
There's probably a race condition in test_live_scanning_multiple_events too.
There was a problem hiding this comment.
I feel like there must be a more reliable way to write these tests to avoid this problem, but I'd leave that for when we're writing tests for channels
Uh oh!
There was an error while loading. Please reload this page.