From f6a0c1fd6370222a38fd72a79ea00f6b208b08c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicki=20K=C5=99=C3=AD=C5=BEek?= Date: Thu, 11 Jun 2026 14:57:58 +0000 Subject: [PATCH 1/2] Add AnsInstance as the ans counterpart of NamedInstance Tests interacting with mock ans servers had to hardcode their IP addresses and open ans.run directly, while named instances already had the NamedInstance abstraction with `.ip`, `.log` and the watch_log_*() helpers. Factor the parts of NamedInstance that are not named-specific into a ServerInstance base class and add an AnsInstance subclass for ans servers, exposed through the `servers` fixture and new ans1-ans11 convenience fixtures. Assisted-by: Claude:claude-fable-5 --- bin/tests/system/conftest.py | 71 +++++++++- bin/tests/system/isctest/instance.py | 197 ++++++++++++++++++--------- 2 files changed, 194 insertions(+), 74 deletions(-) diff --git a/bin/tests/system/conftest.py b/bin/tests/system/conftest.py index 4e58f36c04..b9a4943be9 100644 --- a/bin/tests/system/conftest.py +++ b/bin/tests/system/conftest.py @@ -637,12 +637,16 @@ def servers(system_test_dir): instances = {} for entry in system_test_dir.rglob("*"): if entry.is_dir(): - try: - dir_name = entry.name - instance = isctest.instance.NamedInstance(dir_name) - instances[dir_name] = instance - except ValueError: - continue + dir_name = entry.name + for instance_class in ( + isctest.instance.NamedInstance, + isctest.instance.AnsInstance, + ): + try: + instances[dir_name] = instance_class(dir_name) + break + except ValueError: + continue return instances @@ -699,3 +703,58 @@ def ns10(servers): @pytest.fixture(scope="module") def ns11(servers): return servers["ns11"] + + +@pytest.fixture(scope="module") +def ans1(servers): + return servers["ans1"] + + +@pytest.fixture(scope="module") +def ans2(servers): + return servers["ans2"] + + +@pytest.fixture(scope="module") +def ans3(servers): + return servers["ans3"] + + +@pytest.fixture(scope="module") +def ans4(servers): + return servers["ans4"] + + +@pytest.fixture(scope="module") +def ans5(servers): + return servers["ans5"] + + +@pytest.fixture(scope="module") +def ans6(servers): + return servers["ans6"] + + +@pytest.fixture(scope="module") +def ans7(servers): + return servers["ans7"] + + +@pytest.fixture(scope="module") +def ans8(servers): + return servers["ans8"] + + +@pytest.fixture(scope="module") +def ans9(servers): + return servers["ans9"] + + +@pytest.fixture(scope="module") +def ans10(servers): + return servers["ans10"] + + +@pytest.fixture(scope="module") +def ans11(servers): + return servers["ans11"] diff --git a/bin/tests/system/isctest/instance.py b/bin/tests/system/isctest/instance.py index 2786f66e5d..c6e8301814 100644 --- a/bin/tests/system/isctest/instance.py +++ b/bin/tests/system/isctest/instance.py @@ -14,6 +14,7 @@ from pathlib import Path from typing import NamedTuple +import abc import os import re @@ -39,7 +40,109 @@ class NamedPorts(NamedTuple): ) -class NamedInstance: +class AnsPorts(NamedTuple): + dns: int = 53 + + @staticmethod + def from_env(): + return AnsPorts( + dns=int(os.environ["PORT"]), + ) + + +class ServerInstance(abc.ABC): + """ + Common base class for the server instances used in a system test. + + This class should not be used directly; instead, its subclasses, + `NamedInstance` and `AnsInstance`, should be used. + """ + + @property + @abc.abstractmethod + def log_filename(self) -> str: + """Name of the log file in the instance's directory.""" + + @property + @abc.abstractmethod + def identifier_prefix(self) -> str: + """Directory name prefix used to derive the numeric identifier.""" + + def __init__(self, identifier: str, num: int | None = None) -> None: + """ + `identifier` is the name of the instance's directory + + `num` is optional if the identifier starts with `identifier_prefix` + followed by a number, in which case the number is assumed to be the + numeric identifier; otherwise it must be provided to assign a numeric + identification to the server + """ + self.directory = Path(identifier).absolute() + if not self.directory.is_dir(): + raise ValueError(f"{self.directory} isn't a directory") + self.system_test_name = self.directory.parent.name + + self.identifier = identifier + self.num = self._identifier_to_num(identifier, num) + self.log = TextFile(os.path.join(identifier, self.log_filename)) + + @property + def ip(self) -> str: + """IPv4 address of the instance.""" + return f"10.53.0.{self.num}" + + @classmethod + def _identifier_to_num(cls, identifier: str, num: int | None = None) -> int: + regex_match = re.match( + rf"^{cls.identifier_prefix}(?P[0-9]{{1,2}})$", identifier + ) + if not regex_match: + if num is None: + raise ValueError(f'Can\'t parse numeric identifier from "{identifier}"') + return num + parsed_num = int(regex_match.group("index")) + assert num is None or num == parsed_num, "mismatched num and identifier" + return parsed_num + + def watch_log_from_start( + self, timeout: float = WatchLogFromStart.DEFAULT_TIMEOUT + ) -> WatchLogFromStart: + """ + Return an instance of the `WatchLogFromStart` context manager for this + instance's log file. + """ + return WatchLogFromStart(self.log.path, timeout) + + def watch_log_from_here( + self, timeout: float = WatchLogFromHere.DEFAULT_TIMEOUT + ) -> WatchLogFromHere: + """ + Return an instance of the `WatchLogFromHere` context manager for this + instance's log file. + """ + return WatchLogFromHere(self.log.path, timeout) + + def stop(self, args: list[str] | None = None) -> None: + """Stop the instance.""" + args = args or [] + perl( + f"{os.environ['srcdir']}/stop.pl", + [self.system_test_name, self.identifier] + args, + ) + + def start(self, args: list[str] | None = None) -> None: + """Start the instance.""" + args = args or [] + perl( + f"{os.environ['srcdir']}/start.pl", + [self.system_test_name, self.identifier] + args, + ) + + def __repr__(self): + return self.identifier + + +class NamedInstance(ServerInstance): """ A class representing a `named` instance used in a system test. @@ -51,11 +154,13 @@ class NamedInstance: ``` """ + log_filename = "named.run" + identifier_prefix = "ns" + def __init__( self, identifier: str, num: int | None = None, - ports: NamedPorts | None = None, ) -> None: """ `identifier` is the name of the instance's directory @@ -63,23 +168,9 @@ class NamedInstance: `num` is optional if the identifier is in a form of `ns`, in which case `` is assumed to be numeric identifier; otherwise it must be provided to assign a numeric identification to the server - - `ports` is the `NamedPorts` instance listing the UDP/TCP ports on which - this `named` instance is listening for various types of traffic (both - DNS traffic and RNDC commands). Defaults to ports set by the test - framework. """ - self.directory = Path(identifier).absolute() - if not self.directory.is_dir(): - raise ValueError(f"{self.directory} isn't a directory") - self.system_test_name = self.directory.parent.name - - self.identifier = identifier - self.num = self._identifier_to_num(identifier, num) - if ports is None: - ports = NamedPorts.from_env() - self.ports = ports - self.log = TextFile(os.path.join(identifier, "named.run")) + super().__init__(identifier, num) + self.ports = NamedPorts.from_env() self._rndc_conf = Path("../_common/rndc.conf").absolute() self._rndc = EnvCmd("RNDC", self.rndc_args) @@ -89,22 +180,6 @@ class NamedInstance: """Base arguments for calling RNDC to control the instance.""" return f"-c {self._rndc_conf} -s {self.ip} -p {self.ports.rndc}" - @property - def ip(self) -> str: - """IPv4 address of the instance.""" - return f"10.53.0.{self.num}" - - @staticmethod - def _identifier_to_num(identifier: str, num: int | None = None) -> int: - regex_match = re.match(r"^ns(?P[0-9]{1,2})$", identifier) - if not regex_match: - if num is None: - raise ValueError(f'Can\'t parse numeric identifier from "{identifier}"') - return num - parsed_num = int(regex_match.group("index")) - assert num is None or num == parsed_num, "mismatched num and identifier" - return parsed_num - def rndc(self, command: str, timeout=10, **kwargs) -> CmdResult: """ Send `command` to this named instance using RNDC. Return the server's @@ -139,24 +214,6 @@ class NamedInstance: ) return response - def watch_log_from_start( - self, timeout: float = WatchLogFromStart.DEFAULT_TIMEOUT - ) -> WatchLogFromStart: - """ - Return an instance of the `WatchLogFromStart` context manager for this - `named` instance's log file. - """ - return WatchLogFromStart(self.log.path, timeout) - - def watch_log_from_here( - self, timeout: float = WatchLogFromHere.DEFAULT_TIMEOUT - ) -> WatchLogFromHere: - """ - Return an instance of the `WatchLogFromHere` context manager for this - `named` instance's log file. - """ - return WatchLogFromHere(self.log.path, timeout) - def reconfigure(self, **kwargs) -> CmdResult: """ Reconfigure this named `instance` and wait until reconfiguration is @@ -176,21 +233,25 @@ class NamedInstance: watcher.wait_for_line("all zones loaded") return cmd - def stop(self, args: list[str] | None = None) -> None: - """Stop the instance.""" - args = args or [] - perl( - f"{os.environ['srcdir']}/stop.pl", - [self.system_test_name, self.identifier] + args, - ) - def start(self, args: list[str] | None = None) -> None: - """Start the instance.""" - args = args or [] - perl( - f"{os.environ['srcdir']}/start.pl", - [self.system_test_name, self.identifier] + args, - ) +class AnsInstance(ServerInstance): + """ + A class representing a mock `ans` server instance used in a system test. - def __repr__(self): - return self.identifier + This class is expected to be instantiated as part of the `servers` fixture: + + ```python + def test_foo(servers): + assert "query received" in servers["ans4"].log + ``` + """ + + log_filename = "ans.run" + identifier_prefix = "ans" + + def __init__(self, identifier: str) -> None: + """ + `identifier` is the name of the instance's directory + """ + super().__init__(identifier) + self.ports = AnsPorts.from_env() From 630247e845f133e1fa09e796bf1ecb715b6446f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicki=20K=C5=99=C3=AD=C5=BEek?= Date: Fri, 12 Jun 2026 13:44:10 +0200 Subject: [PATCH 2/2] Use AnsInstance fixture in dispatch test Replace manual ans.run parsing with the ans4 fixture. Assisted-by: Claude:claude-fable-5 --- bin/tests/system/dispatch/tests_tcponly.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/bin/tests/system/dispatch/tests_tcponly.py b/bin/tests/system/dispatch/tests_tcponly.py index 373ee56017..a4d30c3429 100644 --- a/bin/tests/system/dispatch/tests_tcponly.py +++ b/bin/tests/system/dispatch/tests_tcponly.py @@ -27,13 +27,12 @@ pytestmark = pytest.mark.extra_artifacts( ) -def _count_received(path, qname, protocol): +def _count_received(ans, qname, protocol): pattern = Re(rf"Received {escape(qname)}/IN/A .* \({protocol}\)$") - with open(path, encoding="utf-8") as fh: - return sum(1 for line in fh if pattern.search(line.rstrip())) + return len(ans.log.grep(pattern)) -def test_tcponly_fallback(): +def test_tcponly_fallback(ans4): """ A resolver must fall back to TCP after repeated UDP timeouts to the same authoritative server. ans4 drops every UDP query and answers @@ -51,7 +50,7 @@ def test_tcponly_fallback(): ) assert str(rdataset[0]) == "127.0.0.1" - udp = _count_received("ans4/ans.run", "foo.tcp-only", "UDP") - tcp = _count_received("ans4/ans.run", "foo.tcp-only", "TCP") + udp = _count_received(ans4, "foo.tcp-only", "UDP") + tcp = _count_received(ans4, "foo.tcp-only", "TCP") assert udp == 2, f"expected exactly 2 UDP queries, got {udp}" assert tcp == 1, f"expected exactly 1 TCP query, got {tcp}"