Repository: hikari-py/hikari (python)

Issue:
Seperate voice disconnect
Something that just popped my mind, we dont actually have a disconnect a specific channels, we only have a full disconnect from all channels. Should we change disconnect to allow passing a channel to disconnect from and moving the closing all connections to `close`?

(This would be done in another pr, since it is a bit out of scope for this one)

_Originally posted by @davfsa in https://github.com/hikari-py/hikari/pull/629#discussion_r663154067_

Pull request description:
Fix some bugs in voice impl
- Allow disconnecting from voice channel
- ~~Allow moving voice channel in the same server~~

### Checklist
<!-- Make sure to tick all the following boxes by putting an `x` in between (like this `[x]`) -->
- [x] I have run `nox` and all the pipelines have passed.
- [x] I have made unittests according to the code I have added/modified/deleted.

### Related issues
Closes #693


Patch:
diff --git a/dev-requirements.txt b/dev-requirements.txt
index 7ffc5ec7..34832733 100644
--- a/dev-requirements.txt
+++ b/dev-requirements.txt
@@ -9,7 +9,7 @@ mock==4.0.3
 pytest==6.2.4
 pytest-asyncio==0.15.1
 pytest-cov==2.12.1
-pytest-randomly==3.10.0
+pytest-randomly==3.10.1
 
 # Coverage testing.
 coverage==6.0b1
diff --git a/flake8-requirements.txt b/flake8-requirements.txt
index 12c793e9..cb687883 100644
--- a/flake8-requirements.txt
+++ b/flake8-requirements.txt
@@ -9,7 +9,7 @@ flake8-black==0.2.3                     # runs black
 flake8-broken-line==0.3.0               # forbey "\" linebreaks
 flake8-builtins==1.5.3                  # builtin shadowing checks
 flake8-coding==1.3.2                    # coding magic-comment detection
-flake8-comprehensions==3.5.0            # comprehension checks
+flake8-comprehensions==3.6.1            # comprehension checks
 flake8-deprecated==1.3                  # deprecated call checks
 flake8-docstrings==1.6.0                # pydocstyle support
 flake8-executable==2.1.1                # shebangs
diff --git a/hikari/api/voice.py b/hikari/api/voice.py
index aec6aca3..ccd0aee9 100644
--- a/hikari/api/voice.py
+++ b/hikari/api/voice.py
@@ -64,12 +64,19 @@ async def close(self) -> None:
         """
 
     @abc.abstractmethod
-    async def disconnect(self) -> None:
-        """Shut down all connections, waiting for them to terminate.
+    async def disconnect(self, guild: snowflakes.SnowflakeishOr[guilds.PartialGuild]) -> None:
+        """Disconnect from a given guild.
 
-        This will not close the voice component.
+        Parameters
+        ----------
+        guild : hikari.snowflakes.SnowflakeishOr[hikari.guilds.Guild]
+            The guild to disconnect from.
         """
 
+    @abc.abstractmethod
+    async def disconnect_all(self) -> None:
+        """Disconnect all the active voice connections."""
+
     @abc.abstractmethod
     async def connect_to(
         self,
diff --git a/hikari/impl/bot.py b/hikari/impl/bot.py
index a0bf9bbe..ab31b18a 100644
--- a/hikari/impl/bot.py
+++ b/hikari/impl/bot.py
@@ -360,7 +360,9 @@ def get_me(self) -> typing.Optional[users_.OwnUser]:
     async def close(self) -> None:
         """Kill the application by shutting all components down."""
         self._check_if_alive()
+        await self._close()
 
+    async def _close(self) -> None:
         if self._closed_event:  # Closing is in progress from another call, wait for that to complete.
             await self._closed_event.wait()
             return
@@ -658,7 +660,7 @@ def handle_os_interrupt(signum: int, frame: types.FrameType) -> None:
 
         finally:
             try:
-                loop.run_until_complete(self.close())
+                loop.run_until_complete(self._close())
 
                 if close_passed_executor and self._executor is not None:
                     _LOGGER.debug("shutting down executor %s", self._executor)
@@ -829,7 +831,7 @@ async def start(
                     # If any shards stopped silently, we should close.
                     if any(not s.is_alive for s in self._shards.values()):
                         _LOGGER.warning("one of the shards has been manually shut down (no error), will now shut down")
-                        await self.close()
+                        await self._close()
                         return
                     # new window starts.
 
@@ -946,7 +948,7 @@ async def _set_close_flag(self, signame: str, signum: int) -> None:
         # solution.
         _LOGGER.debug("received interrupt %s (%s), will start shutting down shortly", signame, signum)
 
-        await self.close()
+        await self._close()
 
     async def _start_one_shard(
         self,
diff --git a/hikari/impl/voice.py b/hikari/impl/voice.py
index 60809237..219f0a08 100644
--- a/hikari/impl/voice.py
+++ b/hikari/impl/voice.py
@@ -77,22 +77,35 @@ def _check_if_alive(self) -> None:
         if self._is_closing:
             raise errors.ComponentStateConflictError("Component cannot be used while it's closing")
 
-    async def _disconnect(self) -> None:
-        if self._connections:
-            _LOGGER.info("shutting down %s voice connection(s)", len(self._connections))
-            # We rely on the assumption that _on_connection_close will be called here rather than explicitly
-            # emptying self._connections.
-            await asyncio.gather(*(c.disconnect() for c in self._connections.values()))
-
-    async def disconnect(self) -> None:
+    async def disconnect(self, guild: snowflakes.SnowflakeishOr[guilds.PartialGuild]) -> None:
         self._check_if_alive()
-        await self._disconnect()
+        guild_id = snowflakes.Snowflake(guild)
+
+        if guild_id not in self._connections:
+            raise errors.VoiceError("This application doesn't have any active voice connection in this server")
+
+        conn = self._connections[guild_id]
+        # We rely on the assumption that _on_connection_close will be called here rather than explicitly
+        # to remove the connection from self._connections.
+        await conn.disconnect()
+
+    async def _disconnect_all(self) -> None:
+        # We rely on the assumption that _on_connection_close will be called here rather than explicitly
+        # emptying self._connections.
+        await asyncio.gather(*(c.disconnect() for c in self._connections.values()))
+
+    async def disconnect_all(self) -> None:
+        await self._disconnect_all()
 
     async def close(self) -> None:
         self._check_if_alive()
         self._is_closing = True
         self._app.event_manager.unsubscribe(voice_events.VoiceEvent, self._on_voice_event)
-        await self._disconnect()
+
+        if self._connections:
+            _LOGGER.info("shutting down %s active voice connection(s)", len(self._connections))
+            await self._disconnect_all()
+
         self._is_alive = False
         self._is_closing = False
 
@@ -116,34 +129,27 @@ async def connect_to(
     ) -> _VoiceConnectionT:
         self._check_if_alive()
         guild_id = snowflakes.Snowflake(guild)
-        shard_id = snowflakes.calculate_shard_id(self._app, guild_id)
 
         if guild_id in self._connections:
             raise errors.VoiceError(
-                "The bot is already in a voice channel for this guild. Close the other connection first, or "
-                "request that the application moves to the new voice channel instead."
+                "Already in a voice channel for that guild. Disconnect before attempting to connect again"
             )
 
+        shard_id = snowflakes.calculate_shard_id(self._app, guild_id)
         try:
             shard = self._app.shards[shard_id]
         except KeyError:
             raise errors.VoiceError(
-                f"Cannot connect to shard {shard_id}, it is not present in this application."
+                f"Cannot connect to shard {shard_id} as it is not present in this application"
             ) from None
 
-        if not shard.is_alive:
-            # Not sure if I can think of a situation this will happen in... really.
-            # Unless the user sleeps for a bit then tries to connect, and in the mean time the
-            # shard has disconnected.
-            raise errors.VoiceError(f"Cannot connect to shard {shard_id}, the shard is not online.")
-
-        _LOGGER.log(ux.TRACE, "attempting to connect to voice channel %s in %s via shard %s", channel, guild, shard_id)
-
         user = self._app.cache.get_me()
         if not user:
             user = await self._app.rest.fetch_my_user()
 
-        await asyncio.wait_for(shard.update_voice_state(guild, channel, self_deaf=deaf, self_mute=mute), timeout=5.0)
+        _LOGGER.log(ux.TRACE, "attempting to connect to voice channel %s in %s via shard %s", channel, guild, shard_id)
+
+        await shard.update_voice_state(guild, channel, self_deaf=deaf, self_mute=mute)
 
         _LOGGER.log(
             ux.TRACE,
@@ -153,22 +159,19 @@ async def connect_to(
             shard_id,
         )
 
-        state_event, server_event = await asyncio.wait_for(
-            asyncio.gather(
-                # Voice state update:
-                self._app.event_manager.wait_for(
-                    voice_events.VoiceStateUpdateEvent,
-                    timeout=None,
-                    predicate=self._init_state_update_predicate(guild_id, user.id),
-                ),
-                # Server update:
-                self._app.event_manager.wait_for(
-                    voice_events.VoiceServerUpdateEvent,
-                    timeout=None,
-                    predicate=self._init_server_update_predicate(guild_id),
-                ),
+        state_event, server_event = await asyncio.gather(
+            # Voice state update:
+            self._app.event_manager.wait_for(
+                voice_events.VoiceStateUpdateEvent,
+                timeout=None,
+                predicate=self._init_state_update_predicate(guild_id, user.id),
+            ),
+            # Server update:
+            self._app.event_manager.wait_for(
+                voice_events.VoiceServerUpdateEvent,
+                timeout=None,
+                predicate=self._init_server_update_predicate(guild_id),
             ),
-            timeout=10.0,
         )
 
         # We will never receive the first endpoint as `None`
@@ -201,7 +204,11 @@ async def connect_to(
             _LOGGER.debug(
                 "error occurred in initialization, leaving voice channel %s in guild %s again", channel, guild
             )
-            await asyncio.wait_for(shard.update_voice_state(guild, None), timeout=5.0)
+            try:
+                await asyncio.wait_for(shard.update_voice_state(guild, None), timeout=5.0)
+            except asyncio.TimeoutError:
+                pass
+
             raise
 
         self._connections[guild_id] = voice_connection


Test patch:
diff --git a/tests/hikari/impl/test_bot.py b/tests/hikari/impl/test_bot.py
index a0609fd9..9958cee1 100644
--- a/tests/hikari/impl/test_bot.py
+++ b/tests/hikari/impl/test_bot.py
@@ -278,28 +278,37 @@ def test_check_if_alive(self, bot):
         bot._check_if_alive()
 
     @pytest.mark.asyncio()
-    async def test_close_when_already_closed(self, bot, event_manager):
+    async def test_close(self, bot):
+        with mock.patch.object(bot_impl.GatewayBot, "_close") as internal_close:
+            with mock.patch.object(bot_impl.GatewayBot, "_check_if_alive") as check_if_alive:
+                await bot.close()
+
+        check_if_alive.assert_called_once_with()
+        internal_close.assert_awaited_once_with()
+
+    @pytest.mark.asyncio()
+    async def test__close_when_already_closed(self, bot):
         bot._closing_event = None
         bot._closed_event = None
         bot._is_alive = True
 
-        await bot.close()
+        await bot._close()
 
         assert bot._is_alive is True
 
     @pytest.mark.asyncio()
-    async def test_close_when_already_closing(self, bot, event_manager):
+    async def test__close_when_already_closing(self, bot):
         bot._closing_event = object()
         bot._closed_event = mock.AsyncMock()
         bot._is_alive = True
 
-        await bot.close()
+        await bot._close()
 
         bot._closed_event.wait.assert_awaited_once_with()
         assert bot._is_alive is True
 
     @pytest.mark.asyncio()
-    async def test_close(self, bot, event_manager, event_factory, rest, voice, cache):
+    async def test__close(self, bot, event_manager, event_factory, rest, voice, cache):
         def null_call(arg):
             return arg
 
@@ -344,7 +353,7 @@ def assert_awaited_once(self):
         bot._shards = {0: shard0, 1: shard1, 2: shard2}
 
         with stack:
-            await bot.close()
+            await bot._close()
 
         # Events and args
         closing_event.set.assert_called_once_with()
@@ -473,7 +482,7 @@ def test_run_with_asyncio_debug(self, bot):
         stack = contextlib.ExitStack()
         stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "start", new=mock.Mock()))
         stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "join", new=mock.Mock()))
-        stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "close", new=mock.Mock()))
+        stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "_close", new=mock.Mock()))
         loop = stack.enter_context(mock.patch.object(aio, "get_or_make_loop")).return_value
 
         with stack:
@@ -485,7 +494,7 @@ def test_run_with_coroutine_tracking_depth(self, bot):
         stack = contextlib.ExitStack()
         stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "start", new=mock.Mock()))
         stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "join", new=mock.Mock()))
-        stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "close", new=mock.Mock()))
+        stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "_close", new=mock.Mock()))
         stack.enter_context(mock.patch.object(aio, "get_or_make_loop"))
         coroutine_tracking_depth = stack.enter_context(
             mock.patch.object(sys, "set_coroutine_origin_tracking_depth", side_effect=AttributeError)
@@ -500,7 +509,7 @@ def test_run_with_enable_signal_handlers(self, bot):
         stack = contextlib.ExitStack()
         stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "start", new=mock.Mock()))
         stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "join", new=mock.Mock()))
-        stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "close", new=mock.Mock()))
+        stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "_close", new=mock.Mock()))
         stack.enter_context(mock.patch.object(aio, "get_or_make_loop"))
         signal_function = stack.enter_context(
             mock.patch.object(signal, "signal", side_effect=[None, AttributeError, None, AttributeError])
@@ -526,7 +535,7 @@ def raise_signal(*args, **kwargs):
         stack = contextlib.ExitStack()
         stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "start", new=mock.Mock()))
         stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "join", new=raise_signal))
-        stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "close", new=mock.Mock()))
+        stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "_close", new=mock.Mock()))
         stack.enter_context(mock.patch.object(bot_impl, "_LOGGER", isEnabledFor=mock.Mock(return_value=logging)))
         loop = stack.enter_context(mock.patch.object(aio, "get_or_make_loop")).return_value
         set_close_flag = stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "_set_close_flag", new=mock.Mock()))
@@ -543,7 +552,7 @@ def test_run_with_close_passed_executor(self, bot):
         stack = contextlib.ExitStack()
         stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "start", new=mock.Mock()))
         stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "join", new=mock.Mock()))
-        stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "close", new=mock.Mock()))
+        stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "_close", new=mock.Mock()))
         stack.enter_context(mock.patch.object(aio, "get_or_make_loop"))
         executor = mock.Mock()
         bot._executor = executor
@@ -558,7 +567,7 @@ def test_run_with_close_loop(self, bot):
         stack = contextlib.ExitStack()
         stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "start", new=mock.Mock()))
         stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "join", new=mock.Mock()))
-        stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "close", new=mock.Mock()))
+        stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "_close", new=mock.Mock()))
         destroy_loop = stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "_destroy_loop"))
         loop = stack.enter_context(mock.patch.object(aio, "get_or_make_loop")).return_value
 
@@ -581,7 +590,7 @@ def test_run(self, bot):
         stack = contextlib.ExitStack()
         start_function = stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "start", new=mock.Mock()))
         join_function = stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "join", new=mock.Mock()))
-        close_function = stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "close", new=mock.Mock()))
+        close_function = stack.enter_context(mock.patch.object(bot_impl.GatewayBot, "_close", new=mock.Mock()))
         loop = stack.enter_context(mock.patch.object(aio, "get_or_make_loop")).return_value
 
         with stack:
@@ -768,7 +777,7 @@ async def test_request_guild_members(self, bot):
 
     @pytest.mark.asyncio()
     async def test_set_close_flag(self, bot):
-        with mock.patch.object(bot_impl.GatewayBot, "close") as close:
+        with mock.patch.object(bot_impl.GatewayBot, "_close") as close:
             await bot._set_close_flag("Terminated", 15)
 
         close.assert_awaited_once_with()
diff --git a/tests/hikari/impl/test_voice.py b/tests/hikari/impl/test_voice.py
index e83cfd48..be1e5c54 100644
--- a/tests/hikari/impl/test_voice.py
+++ b/tests/hikari/impl/test_voice.py
@@ -24,6 +24,7 @@
 import pytest
 
 from hikari import errors
+from hikari import snowflakes
 from hikari.events import voice_events
 from hikari.impl import voice
 from tests.hikari import hikari_test_helpers
@@ -62,24 +63,60 @@ def test__check_if_alive_when_closing(self, voice_client):
 
     @pytest.mark.asyncio()
     async def test_disconnect(self, voice_client):
+        mock_connection = mock.AsyncMock()
+        mock_connection_2 = mock.AsyncMock()
+        voice_client._connections = {
+            snowflakes.Snowflake(123): mock_connection,
+            snowflakes.Snowflake(5324): mock_connection_2,
+        }
+
+        await voice_client.disconnect(123)
+
+        mock_connection.disconnect.assert_awaited_once_with()
+        mock_connection_2.disconnect.assert_not_called()
+
+    @pytest.mark.asyncio()
+    async def test_disconnect_when_guild_id_not_in_connections(self, voice_client):
         mock_connection = mock.AsyncMock()
         mock_connection_2 = mock.AsyncMock()
         voice_client._connections = {123: mock_connection, 5324: mock_connection_2}
 
-        await voice_client.disconnect()
+        with pytest.raises(errors.VoiceError):
+            await voice_client.disconnect(1234567890)
+
+        mock_connection.disconnect.assert_not_called()
+        mock_connection_2.disconnect.assert_not_called()
+
+    @pytest.mark.asyncio()
+    async def test__disconnect_all(self, voice_client):
+        mock_connection = mock.AsyncMock()
+        mock_connection_2 = mock.AsyncMock()
+        voice_client._connections = {123: mock_connection, 5324: mock_connection_2}
+
+        await voice_client._disconnect_all()
 
         mock_connection.disconnect.assert_awaited_once_with()
         mock_connection_2.disconnect.assert_awaited_once_with()
 
+    @pytest.mark.asyncio()
+    async def test_disconnect_all(self, voice_client):
+        voice_client._disconnect_all = mock.AsyncMock()
+
+        await voice_client.disconnect_all()
+
+        voice_client._disconnect_all.assert_awaited_once_with()
+
     @pytest.mark.asyncio()
     async def test_close(self, voice_client, mock_app):
-        voice_client._disconnect = mock.AsyncMock()
+        voice_client._disconnect_all = mock.AsyncMock()
+        voice_client._connections = {123: None}
+
         await voice_client.close()
 
         mock_app.event_manager.unsubscribe.assert_called_once_with(
             voice_events.VoiceEvent, voice_client._on_voice_event
         )
-        voice_client._disconnect.assert_awaited_once_with()
+        voice_client._disconnect_all.assert_awaited_once_with()
         assert voice_client._is_alive is False
         assert voice_client._is_closing is False
 
@@ -178,13 +215,11 @@ async def test_connect_to_falls_back_to_rest_to_get_own_user(self, voice_client,
 
     @pytest.mark.asyncio()
     async def test_connect_to_when_connection_already_present(self, voice_client, mock_app):
-        mock_app.shard_count = 42
-        voice_client._connections = {123: object()}
+        voice_client._connections = {snowflakes.Snowflake(123): object()}
 
         with pytest.raises(
             errors.VoiceError,
-            match="The bot is already in a voice channel for this guild. Close the other connection first, or "
-            "request that the application moves to the new voice channel instead.",
+            match="Already in a voice channel for that guild. Disconnect before attempting to connect again",
         ):
             await voice_client.connect_to(123, 4532, object())
 
@@ -194,19 +229,10 @@ async def test_connect_to_for_unknown_shard(self, voice_client, mock_app):
         mock_app.shards = {}
 
         with pytest.raises(
-            errors.VoiceError, match="Cannot connect to shard 0, it is not present in this application."
+            errors.VoiceError, match="Cannot connect to shard 0 as it is not present in this application"
         ):
             await voice_client.connect_to(123, 4532, object())
 
-    @pytest.mark.asyncio()
-    async def test_connect_to_for_dead_shard(self, voice_client, mock_app):
-        mock_shard = mock.Mock(is_alive=False)
-        mock_app.shard_count = 42
-        mock_app.shards = {0: mock_shard}
-
-        with pytest.raises(errors.VoiceError, match="Cannot connect to shard 0, the shard is not online."):
-            await voice_client.connect_to(123, 4532, object())
-
     @pytest.mark.asyncio()
     async def test_connect_to_handles_failed_connection_initialise(self, voice_client, mock_app):
         voice_client._init_state_update_predicate = mock.Mock()