Addressing some unit test warnings #3222
Conversation
|
|
|
There was a problem hiding this comment.
Pull request overview
This PR addresses various unit test warnings by improving resource cleanup, updating test fixtures, and pinning dependency versions.
- Properly implements cleanup logic for TCP and FTP contacts to prevent resource warnings
- Updates mock setup for async methods and removes deprecated event_loop fixture usage
- Improves FTP test robustness with better directory handling
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| app/contacts/contact_tcp.py | Adds proper cleanup methods with task cancellation and session closure handling for TCP contact server |
| app/contacts/contact_ftp.py | Simplifies FTP server startup by removing Python version-specific code paths and adds stop method |
| tests/contacts/test_contact_ftp.py | Uses test-specific directory name and shutil.rmtree for non-empty directory cleanup |
| tests/services/test_data_svc.py | Updates async mock setup to use return_value=None instead of async_mock_return helper |
| tests/conftest.py | Removes deprecated event_loop fixture parameter from async fixtures |
| tests/api/v2/test_knowledge.py | Removes deprecated event_loop fixture parameter from async fixture |
| requirements.txt | Pins pyasn1 to version 0.5.x to address related warnings |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.op_loop_task.cancel() | ||
| if self.server_task: | ||
| self.server_task.cancel() | ||
| _ = await asyncio.gather(self.server_task, self.op_loop_task, return_exceptions=True) |
There was a problem hiding this comment.
The asyncio.gather() call will fail with a TypeError if either self.server_task or self.op_loop_task is None. This can occur if stop() is called before start() has been invoked. Consider filtering out None values before passing them to gather(), or checking that both tasks are not None before calling gather().
| _ = await asyncio.gather(self.server_task, self.op_loop_task, return_exceptions=True) | |
| tasks = [t for t in (self.server_task, self.op_loop_task) if t is not None] | |
| if tasks: | |
| _ = await asyncio.gather(*tasks, return_exceptions=True) |
|
|
||
| async def stop(self): | ||
| self.task.cancel() | ||
| await self.server.close() |
There was a problem hiding this comment.
The stop() method will fail with an AttributeError if called when self.server is None. This can occur if stop() is called before start() has been invoked. Consider adding a check to ensure self.server is not None before calling close().
| await self.server.close() | |
| if self.server is not None: | |
| await self.server.close() | |
| self.server = None |
Description
Type of change
How Has This Been Tested?
Ran unit tests locally
Ran manx agent with TCP contact to verify functionality still intact, verified server closed with dead and live manx sessions
Ran sandcat agent with FTP contact to verify functionality still intact
Checklist: