From 9d58e8415c25e0254e61b256603a9fd0314e54c2 Mon Sep 17 00:00:00 2001 From: rina Date: Sat, 27 Jul 2024 12:10:13 +1000 Subject: [PATCH 01/11] tests: add basic end-to-end tests for CLI currently makes use of demo.mcstatus.io as a test server, but it would be good to run a local instance maybe. also adds output tests for --help, allowing this to be used in documentation. --- tests/test_cli.py | 130 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 130 insertions(+) create mode 100644 tests/test_cli.py diff --git a/tests/test_cli.py b/tests/test_cli.py new file mode 100644 index 00000000..c5f3ae09 --- /dev/null +++ b/tests/test_cli.py @@ -0,0 +1,130 @@ +import io +import os +import json +import contextlib +from unittest import mock, skip +from unittest.mock import patch +from pytest import raises + +from mcstatus.__main__ import main as main_under_test + +DEMO_SERVER = "demo.mcstatus.io" + +# XXX: if updating this, be sure to change other occurences of this help text! +EXPECTED_HELP_OUTPUT = """ +usage: mcstatus [-h] [--bedrock] address {ping,status,query,json} ... + +mcstatus provides an easy way to query Minecraft servers for any information they can expose. It provides three modes of access: query, status, ping and json. + +positional arguments: + address The address of the server. + +options: + -h, --help show this help message and exit + --bedrock Specifies that 'address' is a Bedrock server (default: Java). + +commands: + Command to run, defaults to 'status'. + + {ping,status,query,json} + ping Ping server for latency. + status Prints server status. Supported by all Minecraft servers that are version 1.7 or higher. + query Prints detailed server information. Must be enabled in servers' server.properties file. + json Prints server status and query in json. Supported by all Minecraft servers that are version 1.7 or higher. +""" # noqa: E501(line length) + + +@contextlib.contextmanager +def patch_stdout_stderr(): + outpatch = patch("sys.stdout", new=io.StringIO()) + errpatch = patch("sys.stderr", new=io.StringIO()) + with outpatch as out, errpatch as err: + yield out, err + + +# NOTE: for premature exits in argparse, we must catch SystemExit. +# for ordinary exits in the CLI code, we can simply inspect the return value. + + +def test_no_args(): + with patch_stdout_stderr() as (out, _), raises(SystemExit) as exn: + main_under_test([]) + + assert out.getvalue() == "" + assert exn.value.code != 0 + + +def test_help(): + with patch_stdout_stderr() as (out, err), raises(SystemExit) as exn: + main_under_test(["--help"]) + + assert "usage: " in out.getvalue() + assert err.getvalue() == "" + assert exn.value.code == 0 + + +@mock.patch.dict(os.environ, {"COLUMNS": "100000"}) # prevent line-wrapping in --help output +def test_help_matches_recorded_output(): + with patch_stdout_stderr() as (out, err), raises(SystemExit): + main_under_test(["--help"]) + + assert out.getvalue().strip() == EXPECTED_HELP_OUTPUT.strip() + assert err.getvalue() == "" + + +def test_one_argument_is_status(): + with patch_stdout_stderr() as (out, err): + assert main_under_test([DEMO_SERVER]) == 0 + + assert "version:" in out.getvalue() and "players:" in out.getvalue() + assert err.getvalue() == "" + + +def test_status(): + with patch_stdout_stderr() as (out, err): + assert main_under_test([DEMO_SERVER, "status"]) == 0 + + assert "version:" in out.getvalue() and "players:" in out.getvalue() + assert "java" in out.getvalue().lower() + assert err.getvalue() == "" + + +def test_status_bedrock(): + with patch_stdout_stderr() as (out, err): + assert main_under_test([DEMO_SERVER, "--bedrock", "status"]) == 0 + + assert "version:" in out.getvalue() and "players:" in out.getvalue() + assert "bedrock" in out.getvalue().lower() + assert err.getvalue() == "" + + +@skip("demo.mcstatus.io erroneously rejects our query packets. " "see: https://github.com/mcstatus-io/demo-server/issues/1") +def test_query(): + with patch_stdout_stderr() as (out, err): + assert main_under_test([DEMO_SERVER, "query"]) == 0 + + assert "plugins:" in out.getvalue() and "players:" in out.getvalue() + assert err.getvalue() == "" + + +def test_json(): + with patch_stdout_stderr() as (out, err): + assert main_under_test([DEMO_SERVER, "json"]) == 0 + + assert err.getvalue() == "" + data = json.loads(out.getvalue()) + assert data["online"] + assert "status" in data + # TODO: broken for same reason as test_query() + # assert "query" in data + + +def test_ping(): + with patch_stdout_stderr() as (out, err): + assert main_under_test([DEMO_SERVER, "ping"]) == 0 + + assert float(out.getvalue()) > 0 + + # potentially, a warning will be printed to stderr due to: + # https://github.com/py-mine/mcstatus/issues/850 + # assert err.getvalue() == '' From 4576a0325fad66407018f5962faf48b8ff146c0b Mon Sep 17 00:00:00 2001 From: rina Date: Sat, 27 Jul 2024 12:18:37 +1000 Subject: [PATCH 02/11] fix badly formatted string --- tests/test_cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_cli.py b/tests/test_cli.py index c5f3ae09..44cff731 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -98,7 +98,7 @@ def test_status_bedrock(): assert err.getvalue() == "" -@skip("demo.mcstatus.io erroneously rejects our query packets. " "see: https://github.com/mcstatus-io/demo-server/issues/1") +@skip("demo.mcstatus.io erroneously rejects our query packets. see: https://github.com/mcstatus-io/demo-server/issues/1") def test_query(): with patch_stdout_stderr() as (out, err): assert main_under_test([DEMO_SERVER, "query"]) == 0 From 866515ad499c1104386e3912315ea63622157c96 Mon Sep 17 00:00:00 2001 From: rina Date: Sat, 27 Jul 2024 19:36:26 +1000 Subject: [PATCH 03/11] normalise --help output --- tests/test_cli.py | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/tests/test_cli.py b/tests/test_cli.py index 44cff731..f997634d 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -11,6 +11,7 @@ DEMO_SERVER = "demo.mcstatus.io" # XXX: if updating this, be sure to change other occurences of this help text! +# to update, use: `COLUMNS=1000 poetry run mcstatus --help` EXPECTED_HELP_OUTPUT = """ usage: mcstatus [-h] [--bedrock] address {ping,status,query,json} ... @@ -42,6 +43,21 @@ def patch_stdout_stderr(): yield out, err +def normalise_help_output(s: str) -> str: + """ + Normalises the output of `mcstatus --help`, to work around + some discrepancies between Python versions while still retaining + meaningful information for comparison. + """ + + elided = "[...]:" + + s = s.strip() + + # drop lines which end in ":". these argparse section headings vary between python versions. + return "\n".join(ln if not ln.endswith(":") else elided for ln in s.splitlines()) + + # NOTE: for premature exits in argparse, we must catch SystemExit. # for ordinary exits in the CLI code, we can simply inspect the return value. @@ -68,7 +84,7 @@ def test_help_matches_recorded_output(): with patch_stdout_stderr() as (out, err), raises(SystemExit): main_under_test(["--help"]) - assert out.getvalue().strip() == EXPECTED_HELP_OUTPUT.strip() + assert normalise_help_output(out.getvalue()) == normalise_help_output(EXPECTED_HELP_OUTPUT) assert err.getvalue() == "" From 3d0c7a2c017fae6cea55bc0c23043609788d4117 Mon Sep 17 00:00:00 2001 From: PerchunPak Date: Sat, 17 Aug 2024 14:04:05 +0200 Subject: [PATCH 04/11] Make tests fully offline and improve coverage --- .coveragerc | 1 + mcstatus/__main__.py | 27 +++--- tests/test_cli.py | 224 +++++++++++++++++++++++++++++++++++-------- 3 files changed, 199 insertions(+), 53 deletions(-) diff --git a/.coveragerc b/.coveragerc index 51fe600e..b13ac426 100644 --- a/.coveragerc +++ b/.coveragerc @@ -7,3 +7,4 @@ exclude_lines = class .*\bProtocol\): @(abc\.)?abstractmethod raise NotImplementedError + if __name__ == .__main__.: diff --git a/mcstatus/__main__.py b/mcstatus/__main__.py index 1b4e5a5c..73ff8686 100644 --- a/mcstatus/__main__.py +++ b/mcstatus/__main__.py @@ -15,6 +15,20 @@ if TYPE_CHECKING: SupportedServers = JavaServer | BedrockServer +PING_PACKET_FAIL_WARNING = ( + "warning: contacting {address} failed with a 'ping' packet but succeeded with a 'status' packet,\n" + " this is likely a bug in the server-side implementation.\n" + ' (note: ping packet failed due to "{ping_exc}")\n' + " for more details, see: https://mcstatus.readthedocs.io/en/stable/pages/faq/\n" +) + +QUERY_FAIL_WARNING = ( + "The server did not respond to the query protocol." + "\nPlease ensure that the server has enable-query turned on," + " and that the necessary port (same as server-port unless query-port is set) is open in any firewall(s)." + "\nSee https://wiki.vg/Query for further information." +) + def _motd(motd: Motd) -> str: """Formats MOTD for human-readable output, with leading line break @@ -48,10 +62,7 @@ def _ping_with_fallback(server: SupportedServers) -> float: address = f"{server.address.host}:{server.address.port}" print( - f"warning: contacting {address} failed with a 'ping' packet but succeeded with a 'status' packet,\n" - f" this is likely a bug in the server-side implementation.\n" - f' (note: ping packet failed due to "{ping_exc}")\n' - f" for more details, see: https://mcstatus.readthedocs.io/en/stable/pages/faq/\n", + PING_PACKET_FAIL_WARNING.format(address=address, ping_exc=ping_exc), file=sys.stderr, ) @@ -136,13 +147,7 @@ def query_cmd(server: SupportedServers) -> int: try: response = server.query() except socket.timeout: - print( - "The server did not respond to the query protocol." - "\nPlease ensure that the server has enable-query turned on," - " and that the necessary port (same as server-port unless query-port is set) is open in any firewall(s)." - "\nSee https://wiki.vg/Query for further information.", - file=sys.stderr, - ) + print(QUERY_FAIL_WARNING, file=sys.stderr) return 1 print(f"host: {response.raw['hostip']}:{response.raw['hostport']}") diff --git a/tests/test_cli.py b/tests/test_cli.py index f997634d..f09bb117 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1,17 +1,61 @@ import io +import socket + +from mcstatus import JavaServer, BedrockServer +from mcstatus.responses import JavaStatusResponse, BedrockStatusResponse, RawJavaResponse +from mcstatus.querier import QueryResponse + import os import json import contextlib -from unittest import mock, skip +from unittest import mock from unittest.mock import patch -from pytest import raises - -from mcstatus.__main__ import main as main_under_test - -DEMO_SERVER = "demo.mcstatus.io" - -# XXX: if updating this, be sure to change other occurences of this help text! -# to update, use: `COLUMNS=1000 poetry run mcstatus --help` +import pytest + +from mcstatus.__main__ import main as main_under_test, PING_PACKET_FAIL_WARNING, QUERY_FAIL_WARNING + +JAVA_RAW_RESPONSE: RawJavaResponse = { + "players": {"max": 20, "online": 0}, + "version": {"name": "1.8-pre1", "protocol": 44}, + "description": "A Minecraft Server", + "enforcesSecureChat": True, + "favicon": "data:image/png;base64,foo", +} + +QUERY_RAW_RESPONSE = [ + { + "hostname": "A Minecraft Server", + "gametype": "GAME TYPE", + "game_id": "GAME ID", + "version": "1.8", + "plugins": "", + "map": "world", + "numplayers": "3", + "maxplayers": "20", + "hostport": "9999", + "hostip": "192.168.56.1", + }, + ["Dinnerbone", "Djinnibone", "Steve"], +] + +BEDROCK_RAW_RESPONSE = [ + "MCPE", + "§r§4G§r§6a§r§ey§r§2B§r§1o§r§9w§r§ds§r§4e§r§6r", + "422", + "1.18.100500", + "1", + "69", + "3767071975391053022", + "map name here", + "Default", + "1", + "19132", + "-1", + "3", +] + +# NOTE: if updating this, be sure to change other occurrences of this help text! +# to update, use: `COLUMNS=100000 poetry run mcstatus --help` EXPECTED_HELP_OUTPUT = """ usage: mcstatus [-h] [--bedrock] address {ping,status,query,json} ... @@ -43,6 +87,19 @@ def patch_stdout_stderr(): yield out, err +@pytest.fixture +def mock_network_requests(): + with \ + patch("mcstatus.server.JavaServer.lookup", return_value=JavaServer("example.com", port=25565)), \ + patch("mcstatus.server.JavaServer.ping", return_value=0), \ + patch("mcstatus.server.JavaServer.status", return_value=JavaStatusResponse.build(JAVA_RAW_RESPONSE)), \ + patch("mcstatus.server.JavaServer.query", return_value=QueryResponse(*QUERY_RAW_RESPONSE)), \ + patch("mcstatus.server.BedrockServer.lookup", return_value=BedrockServer("example.com", port=25565)), \ + patch("mcstatus.server.BedrockServer.status", return_value=BedrockStatusResponse.build(BEDROCK_RAW_RESPONSE, latency=0) + ): # fmt: skip # multiline with was added in Python 3.10 + yield + + def normalise_help_output(s: str) -> str: """ Normalises the output of `mcstatus --help`, to work around @@ -63,7 +120,7 @@ def normalise_help_output(s: str) -> str: def test_no_args(): - with patch_stdout_stderr() as (out, _), raises(SystemExit) as exn: + with patch_stdout_stderr() as (out, _), pytest.raises(SystemExit) as exn: main_under_test([]) assert out.getvalue() == "" @@ -71,7 +128,7 @@ def test_no_args(): def test_help(): - with patch_stdout_stderr() as (out, err), raises(SystemExit) as exn: + with patch_stdout_stderr() as (out, err), pytest.raises(SystemExit) as exn: main_under_test(["--help"]) assert "usage: " in out.getvalue() @@ -81,66 +138,149 @@ def test_help(): @mock.patch.dict(os.environ, {"COLUMNS": "100000"}) # prevent line-wrapping in --help output def test_help_matches_recorded_output(): - with patch_stdout_stderr() as (out, err), raises(SystemExit): + with patch_stdout_stderr() as (out, err), pytest.raises(SystemExit): main_under_test(["--help"]) assert normalise_help_output(out.getvalue()) == normalise_help_output(EXPECTED_HELP_OUTPUT) assert err.getvalue() == "" -def test_one_argument_is_status(): +def test_one_argument_is_status(mock_network_requests): with patch_stdout_stderr() as (out, err): - assert main_under_test([DEMO_SERVER]) == 0 - - assert "version:" in out.getvalue() and "players:" in out.getvalue() + assert main_under_test(["example.com"]) == 0 + + assert ( + "version: Java 1.8-pre1 (protocol 44)\n" + "motd: \x1b[0mA Minecraft Server\x1b[0m\n" + "players: 0/20 No players online\n" + "ping: 0.00 ms\n" + ) == out.getvalue() assert err.getvalue() == "" -def test_status(): +def test_status(mock_network_requests): with patch_stdout_stderr() as (out, err): - assert main_under_test([DEMO_SERVER, "status"]) == 0 - - assert "version:" in out.getvalue() and "players:" in out.getvalue() - assert "java" in out.getvalue().lower() + assert main_under_test(["example.com", "status"]) == 0 + + assert ( + "version: Java 1.8-pre1 (protocol 44)\n" + "motd: \x1b[0mA Minecraft Server\x1b[0m\n" + "players: 0/20 No players online\n" + "ping: 0.00 ms\n" + ) == out.getvalue() assert err.getvalue() == "" -def test_status_bedrock(): +def test_status_bedrock(mock_network_requests): with patch_stdout_stderr() as (out, err): - assert main_under_test([DEMO_SERVER, "--bedrock", "status"]) == 0 + assert main_under_test(["example.com", "--bedrock", "status"]) == 0 + + assert ( + "version: Bedrock 1.18.100500 (protocol 422)\n" + "motd: \x1b[0m\x1b[0m\x1b[0m\x1b[38;2;170;0;0mG\x1b[0m\x1b[0m\x1b[38;2;255;170;0ma\x1b[0m\x1b[0m\x1b[38;2;255;255;85m" + "y\x1b[0m\x1b[0m\x1b[38;2;0;170;0mB\x1b[0m\x1b[0m\x1b[38;2;0;0;170mo\x1b[0m\x1b[0m\x1b[38;2;85;85;255mw\x1b[0m\x1b[0m" + "\x1b[38;2;255;85;255ms\x1b[0m\x1b[0m\x1b[38;2;170;0;0me\x1b[0m\x1b[0m\x1b[38;2;255;170;0mr\x1b[0m\n" + "players: 1/69\n" + "ping: 0.00 ms\n" + ) == out.getvalue() + assert err.getvalue() == "" - assert "version:" in out.getvalue() and "players:" in out.getvalue() - assert "bedrock" in out.getvalue().lower() + +def test_status_offline(mock_network_requests): + with patch_stdout_stderr() as (out, err), patch("mcstatus.server.JavaServer.status", side_effect=socket.timeout): + assert main_under_test(["example.com", "status"]) == 1 + + assert out.getvalue() == "" + assert err.getvalue() == "Error: \n" + + +def test_query(mock_network_requests): + with patch_stdout_stderr() as (out, err): + assert main_under_test(["example.com", "query"]) == 0 + + assert ( + "host: 192.168.56.1:9999\n" + "software: Java 1.8 vanilla\n" + "motd: \x1b[0mA Minecraft Server\x1b[0m\n" + "plugins: []\n" + "players: 3/20 ['Dinnerbone', 'Djinnibone', 'Steve']\n" + ) == out.getvalue() assert err.getvalue() == "" -@skip("demo.mcstatus.io erroneously rejects our query packets. see: https://github.com/mcstatus-io/demo-server/issues/1") -def test_query(): +def test_query_offline(mock_network_requests): + with patch_stdout_stderr() as (out, err), patch("mcstatus.server.JavaServer.query", side_effect=socket.timeout): + assert main_under_test(["example.com", "query"]) != 0 + + assert out.getvalue() == "" + assert err.getvalue() == QUERY_FAIL_WARNING + "\n" + + +def test_json(mock_network_requests): with patch_stdout_stderr() as (out, err): - assert main_under_test([DEMO_SERVER, "query"]) == 0 + assert main_under_test(["example.com", "json"]) == 0 - assert "plugins:" in out.getvalue() and "players:" in out.getvalue() + data = json.loads(out.getvalue()) + assert data == { + "online": True, + "kind": "Java", + "status": { + "players": {"online": 0, "max": 20, "sample": None}, + "version": {"name": "1.8-pre1", "protocol": 44}, + "motd": "A Minecraft Server", + "latency": 0, + "raw": { + "players": {"max": 20, "online": 0}, + "version": {"name": "1.8-pre1", "protocol": 44}, + "description": "A Minecraft Server", + "enforcesSecureChat": True, + "favicon": "data:image/png;base64,foo", + }, + "enforces_secure_chat": True, + "icon": "data:image/png;base64,foo", + "forge_data": None, + }, + "query": { + "ip": "192.168.56.1", + "port": "9999", + "map": "world", + "plugins": [], + "raw": { + "hostname": "A Minecraft Server", + "gametype": "GAME TYPE", + "game_id": "GAME ID", + "version": "1.8", + "plugins": "", + "map": "world", + "numplayers": "3", + "maxplayers": "20", + "hostport": "9999", + "hostip": "192.168.56.1", + }, + }, + } assert err.getvalue() == "" -def test_json(): +def test_ping(mock_network_requests): with patch_stdout_stderr() as (out, err): - assert main_under_test([DEMO_SERVER, "json"]) == 0 + assert main_under_test(["example.com", "ping"]) == 0 + assert float(out.getvalue()) == 0 assert err.getvalue() == "" - data = json.loads(out.getvalue()) - assert data["online"] - assert "status" in data - # TODO: broken for same reason as test_query() - # assert "query" in data -def test_ping(): +def test_ping_bedrock(mock_network_requests): with patch_stdout_stderr() as (out, err): - assert main_under_test([DEMO_SERVER, "ping"]) == 0 + assert main_under_test(["example.com", "--bedrock", "ping"]) == 0 + + assert float(out.getvalue()) == 0 + assert err.getvalue() == "" + - assert float(out.getvalue()) > 0 +def test_ping_server_doesnt_support(mock_network_requests): + with patch_stdout_stderr() as (out, err), patch("mcstatus.server.JavaServer.ping", side_effect=TimeoutError("timeout")): + assert main_under_test(["example.com", "ping"]) == 0 - # potentially, a warning will be printed to stderr due to: - # https://github.com/py-mine/mcstatus/issues/850 - # assert err.getvalue() == '' + assert float(out.getvalue()) == 0 + assert err.getvalue() == PING_PACKET_FAIL_WARNING.format(address="example.com:25565", ping_exc="timeout") + "\n" From fee43981ceafa8a39806a4938dcd95052c505fdd Mon Sep 17 00:00:00 2001 From: PerchunPak Date: Fri, 23 Aug 2024 15:21:17 +0200 Subject: [PATCH 05/11] Address review --- .coveragerc | 2 +- tests/test_cli.py | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/.coveragerc b/.coveragerc index b13ac426..c8646d9c 100644 --- a/.coveragerc +++ b/.coveragerc @@ -7,4 +7,4 @@ exclude_lines = class .*\bProtocol\): @(abc\.)?abstractmethod raise NotImplementedError - if __name__ == .__main__.: + if __name__ == "__main__": diff --git a/tests/test_cli.py b/tests/test_cli.py index f09bb117..545b9ee0 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -94,6 +94,7 @@ def mock_network_requests(): patch("mcstatus.server.JavaServer.ping", return_value=0), \ patch("mcstatus.server.JavaServer.status", return_value=JavaStatusResponse.build(JAVA_RAW_RESPONSE)), \ patch("mcstatus.server.JavaServer.query", return_value=QueryResponse(*QUERY_RAW_RESPONSE)), \ + patch("mcstatus.server.BedrockServer.ping", return_value=123), \ patch("mcstatus.server.BedrockServer.lookup", return_value=BedrockServer("example.com", port=25565)), \ patch("mcstatus.server.BedrockServer.status", return_value=BedrockStatusResponse.build(BEDROCK_RAW_RESPONSE, latency=0) ): # fmt: skip # multiline with was added in Python 3.10 @@ -120,10 +121,11 @@ def normalise_help_output(s: str) -> str: def test_no_args(): - with patch_stdout_stderr() as (out, _), pytest.raises(SystemExit) as exn: + with patch_stdout_stderr() as (out, err), pytest.raises(SystemExit) as exn: main_under_test([]) assert out.getvalue() == "" + assert "usage: " in err.getvalue() assert exn.value.code != 0 @@ -274,7 +276,7 @@ def test_ping_bedrock(mock_network_requests): with patch_stdout_stderr() as (out, err): assert main_under_test(["example.com", "--bedrock", "ping"]) == 0 - assert float(out.getvalue()) == 0 + assert float(out.getvalue()) == 123 assert err.getvalue() == "" From b62a763f5a4d46245b8133829b56b323947cec8c Mon Sep 17 00:00:00 2001 From: PerchunPak Date: Fri, 23 Aug 2024 15:22:45 +0200 Subject: [PATCH 06/11] Fix tests --- tests/test_cli.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/test_cli.py b/tests/test_cli.py index 545b9ee0..dfd8624d 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -94,9 +94,8 @@ def mock_network_requests(): patch("mcstatus.server.JavaServer.ping", return_value=0), \ patch("mcstatus.server.JavaServer.status", return_value=JavaStatusResponse.build(JAVA_RAW_RESPONSE)), \ patch("mcstatus.server.JavaServer.query", return_value=QueryResponse(*QUERY_RAW_RESPONSE)), \ - patch("mcstatus.server.BedrockServer.ping", return_value=123), \ patch("mcstatus.server.BedrockServer.lookup", return_value=BedrockServer("example.com", port=25565)), \ - patch("mcstatus.server.BedrockServer.status", return_value=BedrockStatusResponse.build(BEDROCK_RAW_RESPONSE, latency=0) + patch("mcstatus.server.BedrockServer.status", return_value=BedrockStatusResponse.build(BEDROCK_RAW_RESPONSE, latency=123) ): # fmt: skip # multiline with was added in Python 3.10 yield @@ -183,7 +182,7 @@ def test_status_bedrock(mock_network_requests): "y\x1b[0m\x1b[0m\x1b[38;2;0;170;0mB\x1b[0m\x1b[0m\x1b[38;2;0;0;170mo\x1b[0m\x1b[0m\x1b[38;2;85;85;255mw\x1b[0m\x1b[0m" "\x1b[38;2;255;85;255ms\x1b[0m\x1b[0m\x1b[38;2;170;0;0me\x1b[0m\x1b[0m\x1b[38;2;255;170;0mr\x1b[0m\n" "players: 1/69\n" - "ping: 0.00 ms\n" + "ping: 123.00 ms\n" ) == out.getvalue() assert err.getvalue() == "" From 67a260863689712adc20fa2336d6ad793f864713 Mon Sep 17 00:00:00 2001 From: PerchunPak Date: Wed, 28 Aug 2024 10:18:00 +0200 Subject: [PATCH 07/11] Use repr for common errors --- mcstatus/__main__.py | 2 +- tests/test_cli.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/mcstatus/__main__.py b/mcstatus/__main__.py index 73ff8686..20cb03f3 100644 --- a/mcstatus/__main__.py +++ b/mcstatus/__main__.py @@ -194,7 +194,7 @@ def main(argv: list[str] = sys.argv[1:]) -> int: return args.func(server) except (socket.timeout, socket.gaierror, dns.resolver.NoNameservers, ConnectionError) as e: # catch and hide traceback for expected user-facing errors - print(f"Error: {e}", file=sys.stderr) + print(f"Error: {e!r}", file=sys.stderr) return 1 diff --git a/tests/test_cli.py b/tests/test_cli.py index dfd8624d..a8040808 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -192,7 +192,7 @@ def test_status_offline(mock_network_requests): assert main_under_test(["example.com", "status"]) == 1 assert out.getvalue() == "" - assert err.getvalue() == "Error: \n" + assert err.getvalue() == "Error: TimeoutError()\n" def test_query(mock_network_requests): From 42e63e1b30afa9b3e0f4b8d8720dc2e78bd91d23 Mon Sep 17 00:00:00 2001 From: PerchunPak Date: Wed, 28 Aug 2024 10:19:39 +0200 Subject: [PATCH 08/11] Fix CI --- tests/test_cli.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/test_cli.py b/tests/test_cli.py index a8040808..bb9f3177 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -95,7 +95,9 @@ def mock_network_requests(): patch("mcstatus.server.JavaServer.status", return_value=JavaStatusResponse.build(JAVA_RAW_RESPONSE)), \ patch("mcstatus.server.JavaServer.query", return_value=QueryResponse(*QUERY_RAW_RESPONSE)), \ patch("mcstatus.server.BedrockServer.lookup", return_value=BedrockServer("example.com", port=25565)), \ - patch("mcstatus.server.BedrockServer.status", return_value=BedrockStatusResponse.build(BEDROCK_RAW_RESPONSE, latency=123) + patch("mcstatus.server.BedrockServer.status", return_value=( + BedrockStatusResponse.build(BEDROCK_RAW_RESPONSE, latency=123) + ) ): # fmt: skip # multiline with was added in Python 3.10 yield From 65ba25bfd93abdc891445edb107a28f31dcf15e5 Mon Sep 17 00:00:00 2001 From: PerchunPak Date: Wed, 28 Aug 2024 10:23:10 +0200 Subject: [PATCH 09/11] Raise `TimeoutError` instead of `socket.timeout` in mock In 3.12 `socket.timeout` is an alias to `TimeoutError`, but in previous versions they are different. --- tests/test_cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_cli.py b/tests/test_cli.py index bb9f3177..db25f482 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -190,7 +190,7 @@ def test_status_bedrock(mock_network_requests): def test_status_offline(mock_network_requests): - with patch_stdout_stderr() as (out, err), patch("mcstatus.server.JavaServer.status", side_effect=socket.timeout): + with patch_stdout_stderr() as (out, err), patch("mcstatus.server.JavaServer.status", side_effect=TimeoutError): assert main_under_test(["example.com", "status"]) == 1 assert out.getvalue() == "" From f5499c72a793f3d27439b43ce4c4485297addfd5 Mon Sep 17 00:00:00 2001 From: PerchunPak Date: Wed, 28 Aug 2024 10:26:35 +0200 Subject: [PATCH 10/11] Catch `TimeoutError` in CLI as one of the common exceptions --- mcstatus/__main__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mcstatus/__main__.py b/mcstatus/__main__.py index 20cb03f3..66e39a85 100644 --- a/mcstatus/__main__.py +++ b/mcstatus/__main__.py @@ -192,7 +192,7 @@ def main(argv: list[str] = sys.argv[1:]) -> int: try: server = lookup(args.address) return args.func(server) - except (socket.timeout, socket.gaierror, dns.resolver.NoNameservers, ConnectionError) as e: + except (socket.timeout, socket.gaierror, dns.resolver.NoNameservers, ConnectionError, TimeoutError) as e: # catch and hide traceback for expected user-facing errors print(f"Error: {e!r}", file=sys.stderr) return 1 From c01c7caa53eba162074b2ce8ee122446e85443f3 Mon Sep 17 00:00:00 2001 From: PerchunPak Date: Thu, 12 Sep 2024 19:15:36 +0200 Subject: [PATCH 11/11] Clarify comment as requested in review --- tests/test_cli.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_cli.py b/tests/test_cli.py index db25f482..22ad428f 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -114,6 +114,7 @@ def normalise_help_output(s: str) -> str: s = s.strip() # drop lines which end in ":". these argparse section headings vary between python versions. + # it is just a small style change, so it doesn't matter so much to do `sys.version_info` check return "\n".join(ln if not ln.endswith(":") else elided for ln in s.splitlines())