Fix RuntimeWarning and DeprecationWarning in test suite#62
Conversation
…ad of find_executable Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
| return # Let async wrapper handle shutdown | ||
| self.shutdown(save=True, now=True, force=True) | ||
| # Use execute_command to avoid async/sync confusion | ||
| self.execute_command('SHUTDOWN', 'SAVE', 'NOW', 'FORCE') |
There was a problem hiding this comment.
@copilot why not verify in runtime?
# Check if it's a coroutine function
if inspect.iscoroutinefunction(shutdown):
await shutdown()
else:
shutdown()
There was a problem hiding this comment.
Good suggestion! I've implemented the runtime check approach in commit ced665b.
The code now checks inspect.iscoroutinefunction(self.shutdown) at runtime:
- If it's a coroutine function (async), uses
execute_command(always synchronous) - Otherwise, calls
self.shutdown()with the original parameters
This preserves the original method signature while handling both sync and async cases gracefully. All tests pass without warnings.
Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
| # Check if shutdown is a coroutine function to avoid async/sync confusion | ||
| if hasattr(self, 'shutdown') and inspect.iscoroutinefunction(self.shutdown): | ||
| # If it's async, use execute_command which is always synchronous | ||
| self.execute_command('SHUTDOWN', 'SAVE', 'NOW', 'FORCE') |
There was a problem hiding this comment.
@copilot don't you think it's better to use asyncio when calling the async API?
asyncio.run(self.shutdown(save=True, now=True, force=True))
There was a problem hiding this comment.
You're right! Using asyncio.run() is the proper way to handle the async shutdown. I've updated the code in commit 09bd73c to use asyncio.run(self.shutdown(save=True, now=True, force=True)) instead of falling back to execute_command.
This properly awaits the async method when detected, maintaining the original method signature and behavior. All tests pass without warnings.
Co-authored-by: gkorland <753206+gkorland@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Addresses warnings emitted during test runs by correctly awaiting async shutdown logic and replacing a deprecated executable lookup API.
Changes:
- Update
redislite/client.pycleanup flow to detect asyncshutdown()and await it viaasyncio. - Replace deprecated
distutils.spawn.find_executableusage withshutil.whichin debug info collection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| redislite/client.py | Detects coroutine shutdown and runs/awaits it to prevent “coroutine was never awaited” warnings. |
| redislite/debug.py | Switches to shutil.which to eliminate find_executable deprecation warnings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # If it's async, use asyncio.run to properly await it | ||
| asyncio.run(self.shutdown(save=True, now=True, force=True)) |
There was a problem hiding this comment.
asyncio.run(...) will raise RuntimeError if _cleanup() executes while an event loop is already running (e.g., in async test environments). Consider handling the 'running loop' case by using asyncio.get_running_loop() and scheduling the coroutine (e.g., loop.create_task(...)) when a loop is active, and only falling back to asyncio.run(coro) when there is no running loop.
| # If it's async, use asyncio.run to properly await it | |
| asyncio.run(self.shutdown(save=True, now=True, force=True)) | |
| # If it's async, run it safely depending on whether an event loop is running | |
| coro = self.shutdown(save=True, now=True, force=True) | |
| try: | |
| loop = asyncio.get_running_loop() | |
| except RuntimeError: | |
| # No running event loop; safe to use asyncio.run | |
| asyncio.run(coro) | |
| else: | |
| # Event loop already running; schedule the coroutine on it | |
| loop.create_task(coro) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Check if shutdown is a coroutine function to avoid async/sync confusion | ||
| if hasattr(self, 'shutdown') and inspect.iscoroutinefunction(self.shutdown): | ||
| # If it's async, use asyncio.run to properly await it | ||
| asyncio.run(self.shutdown(save=True, now=True, force=True)) | ||
| else: |
There was a problem hiding this comment.
asyncio.run(...) will raise RuntimeError if _cleanup() is invoked while an event loop is already running (common in async applications / notebooks). Since the exception isn’t caught (only redis.RedisError is handled), cleanup can abort before the fallback SIGTERM/SIGKILL logic runs, potentially leaving a redis-server process behind. Consider detecting an active loop (asyncio.get_running_loop()), and either scheduling the coroutine safely or falling back to the existing signal-based shutdown path, and/or catching RuntimeError so cleanup still proceeds.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #62 +/- ##
==========================================
- Coverage 85.13% 83.85% -1.29%
==========================================
Files 8 8
Lines 572 576 +4
Branches 69 70 +1
==========================================
- Hits 487 483 -4
- Misses 67 72 +5
- Partials 18 21 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Two warnings were appearing during test execution:
RuntimeWarning: coroutine 'AsyncManagementCommands.shutdown' was never awaitedinclient.py:112DeprecationWarning: Use shutil.which instead of find_executableindebug.py:61Changes
redislite/client.py: Runtime detection for async/sync shutdownimport asyncioandimport inspectto handle async/sync shutdown methodsinspect.iscoroutinefunction(self.shutdown):shutdownis a coroutine function (async), usesasyncio.run(self.shutdown(save=True, now=True, force=True))to properly await the coroutineshutdownis a normal function (sync), callsself.shutdown(save=True, now=True, force=True)as originally intendedasyncio.run()redislite/debug.py: Replace deprecatedfind_executablewithshutil.whichdistutils.spawn.find_executabletoshutil.whichdistutils.spawn.find_executabledeprecated since Python 3.3Warning
<issue_title>Warnings in tests</issue_title>
<issue_description>```
tests/test_client.py::TestRedisliteClient::test_redis_log_attribute
/home/runner/work/falkordblite/falkordblite/redislite/client.py:112: RuntimeWarning: coroutine 'AsyncManagementCommands.shutdown' was never awaited
self.shutdown(save=True, now=True, force=True)
Enable tracemalloc to get traceback where the object was allocated.
See https://docs.pytest.org/en/stable/how-to/capture-warnings.html#resource-warnings for more info.
tests/test_configuration.py::TestRedisliteConfiguration::test_debug
tests/test_debug.py::TestRedisliteDebug::test_debug
/home/runner/work/falkordblite/falkordblite/redislite/debug.py:61: DeprecationWarning: Use shutil.which instead of find_executable
redis_server = find_executable('redis-server')
-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
<issue_title>Warnings in tests</issue_title>
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.