WIP: Add test for filter calls between fetch() #175
Conversation
Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
By using non-dict-assigning dispatch update flow. When fetch() which runs periodically would assign a fresh map to `self._dispatches` the filter would from the non operate on an old non-updated map. Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
github-actions
bot
added
part:docs
There was a problem hiding this comment.
Pull Request Overview
This PR adds a test to verify that the dispatch filtering mechanism correctly handles overlapping dispatches that are created between fetch operations. The test specifically verifies that when two dispatches overlap in time but are created at different points relative to the fetch cycle, the merge strategy correctly filters events and maintains proper state.
- Adds a comprehensive test for sequential overlapping dispatches between fetch operations
- Updates the merge strategy to use consistent time references and improved logging
- Fixes fetch logic to properly handle dispatch updates and deletions
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_frequenz_dispatch.py | Adds new test case for overlapping dispatches between fetch operations |
| src/frequenz/dispatch/_merge_strategies.py | Updates merge filter to use consistent time reference and improved logging |
| src/frequenz/dispatch/_dispatch.py | Adds started_at method for time-aware dispatch state checking |
| src/frequenz/dispatch/_bg_service.py | Fixes fetch logic to properly handle dispatch updates and scheduling |
| src/frequenz/dispatch/_actor_dispatcher.py | Improves logging for actor stop operations |
| pyproject.toml | Updates frequenz-client-dispatch dependency version |
| RELEASE_NOTES.md | Updates release notes with bug fixes |
Comments suppressed due to low confidence (1)
tests/test_frequenz_dispatch.py:759
- The test expects a TimeoutError but doesn't specify a timeout parameter for the receiver.receive() call. This could cause the test to hang indefinitely if the expected timeout doesn't occur.
with pytest.raises(asyncio.TimeoutError):
| import logging | ||
|
|
There was a problem hiding this comment.
Import statements should be at the top of the file, not inside functions. Move this import to the top with other imports.
| import logging |
Copilot uses AI. Check for mistakes.
| await client.create(**to_create_params(microgrid_id, dispatch1)) | ||
|
|
||
| timer = Timer(timedelta(seconds=100), SkipMissedAndResync(), auto_start=False) | ||
| await service._fetch(timer) # pylint: disable=protected-access |
There was a problem hiding this comment.
[nitpick] Accessing protected methods in tests can make tests brittle. Consider exposing a public method for testing or using dependency injection to make the fetch operation testable.
Copilot uses AI. Check for mistakes.
| import logging | ||
|
|
||
| logging.debug("We see: %s", service._dispatches) |
There was a problem hiding this comment.
Debug logging and protected member access should be removed from test code. This appears to be leftover debugging code.
| import logging | |
| logging.debug("We see: %s", service._dispatches) |
Copilot uses AI. Check for mistakes.
| await asyncio.sleep(1) | ||
|
|
||
| with pytest.raises(asyncio.TimeoutError): | ||
| logging.debug("Wait for now starts %s", _now()) |
There was a problem hiding this comment.
Debug logging should be removed from test code. This appears to be leftover debugging code.
| logging.debug("Wait for now starts %s", _now()) |
Copilot uses AI. Check for mistakes.
| # Delete old dispatches | ||
| for dispatch_id in old_dispatches: | ||
| if dispatch_id in new_dispatches: | ||
| continue | ||
|
|
||
| dispatch = self._dispatches.pop(dispatch_id) | ||
| _logger.debug("Deleted dispatch: %s", dispatch) | ||
| await self._lifecycle_events_tx.send(Deleted(dispatch=dispatch)) | ||
| await self._update_dispatch_schedule_and_notify(None, dispatch, timer) | ||
|
|
||
| # Set deleted only here as it influences the result of dispatch.started | ||
| # which is used in above in _running_state_change | ||
| dispatch._set_deleted() # pylint: disable=protected-access | ||
| await self._lifecycle_events_tx.send(Deleted(dispatch=dispatch)) | ||
| self._dispatches.update(new_dispatches) | ||
|
|
There was a problem hiding this comment.
The dispatches dictionary is updated after processing deletions, but the deletion logic uses the old dispatches dictionary. This could lead to inconsistent state if the same dispatch ID exists in both old and new dispatches.
Copilot uses AI. Check for mistakes.
ONLY LOOK AT THE LAST COMMIT. The rest is in another PR