From 67b19697b3f44ef7760d6a26511576ba30ed6a12 Mon Sep 17 00:00:00 2001 From: Cornelius Riemenschneider Date: Wed, 21 Aug 2024 22:56:45 +0200 Subject: [PATCH 1/2] Loadgroup: Don't modify test nodeids any longer. The loadgroup scheduler used to modify the nodeids of tests that were placed manually in a specific scope. This was used as a side-channel from test collection (on the pytest-running process) to the worker process. Instead, make that side-channel explicit by passing a parameter around. That's pretty ugly, but no less ugly than modifying and parsing (!) node IDs later on. --- src/xdist/dsession.py | 6 +++++- src/xdist/remote.py | 32 +++++++++++++------------------- src/xdist/scheduler/loadgroup.py | 7 +++---- src/xdist/workermanage.py | 7 ++++++- 4 files changed, 27 insertions(+), 25 deletions(-) diff --git a/src/xdist/dsession.py b/src/xdist/dsession.py index 62079a28..f105dc75 100644 --- a/src/xdist/dsession.py +++ b/src/xdist/dsession.py @@ -272,7 +272,10 @@ def pytest_terminal_summary(self, terminalreporter: Any) -> None: terminalreporter.write_sep("=", f"xdist: {self._summary_report}") def worker_collectionfinish( - self, node: WorkerController, ids: Sequence[str] + self, + node: WorkerController, + ids: Sequence[str], + loadgroup_scopes: dict[str, str], ) -> None: """Worker has finished test collection. @@ -290,6 +293,7 @@ def worker_collectionfinish( assert self._session is not None self._session.testscollected = len(ids) assert self.sched is not None + self.sched.loadgroup_scopes = loadgroup_scopes self.sched.add_node_collection(node, ids) if self.terminal: self.trdist.setstatus( diff --git a/src/xdist/remote.py b/src/xdist/remote.py index dd1f9883..d64661d9 100644 --- a/src/xdist/remote.py +++ b/src/xdist/remote.py @@ -201,30 +201,25 @@ def run_one_test(self) -> None: "runtest_protocol_complete", item_index=self.item_index, duration=duration ) - def pytest_collection_modifyitems( - self, - config: pytest.Config, - items: list[pytest.Item], - ) -> None: - # add the group name to nodeid as suffix if --dist=loadgroup - if config.getvalue("loadgroup"): - for item in items: - mark = item.get_closest_marker("xdist_group") - if not mark: - continue - gname = ( - mark.args[0] - if len(mark.args) > 0 - else mark.kwargs.get("name", "default") - ) - item._nodeid = f"{item.nodeid}@{gname}" - @pytest.hookimpl def pytest_collection_finish(self, session: pytest.Session) -> None: + # collect the scope for each node for --dist=loadgroup + loadgroup_scopes = {} + for item in session.items: + mark = item.get_closest_marker("xdist_group") + if not mark: + continue + gname = ( + mark.args[0] + if len(mark.args) > 0 + else mark.kwargs.get("name", "default") + ) + loadgroup_scopes[item.nodeid] = gname self.sendevent( "collectionfinish", topdir=str(self.config.rootpath), ids=[item.nodeid for item in session.items], + loadgroup_scopes=loadgroup_scopes, ) @pytest.hookimpl @@ -356,7 +351,6 @@ def getinfodict() -> WorkerInfo: def setup_config(config: pytest.Config, basetemp: str | None) -> None: - config.option.loadgroup = config.getvalue("dist") == "loadgroup" config.option.looponfail = False config.option.usepdb = False config.option.dist = "no" diff --git a/src/xdist/scheduler/loadgroup.py b/src/xdist/scheduler/loadgroup.py index 798c7128..0cc1cf0d 100644 --- a/src/xdist/scheduler/loadgroup.py +++ b/src/xdist/scheduler/loadgroup.py @@ -52,8 +52,7 @@ def _split_scope(self, nodeid: str) -> str: gname gname """ - if nodeid.rfind("@") > nodeid.rfind("]"): - # check the index of ']' to avoid the case: parametrize mark value has '@' - return nodeid.split("@")[-1] + if nodeid in self.loadgroup_scopes: + return self.loadgroup_scopes[nodeid] else: - return nodeid + return super()._split_scope(nodeid) diff --git a/src/xdist/workermanage.py b/src/xdist/workermanage.py index 44d1be4c..cec1f039 100644 --- a/src/xdist/workermanage.py +++ b/src/xdist/workermanage.py @@ -418,7 +418,12 @@ def process_from_remote( rep.item_index = item_index self.notify_inproc(eventname, node=self, rep=rep) elif eventname == "collectionfinish": - self.notify_inproc(eventname, node=self, ids=kwargs["ids"]) + self.notify_inproc( + eventname, + node=self, + ids=kwargs["ids"], + loadgroup_scopes=kwargs["loadgroup_scopes"], + ) elif eventname == "runtest_protocol_complete": self.notify_inproc(eventname, node=self, **kwargs) elif eventname == "unscheduled": From d469737893fd9404b639a2b848cfa95120b633ab Mon Sep 17 00:00:00 2001 From: Cornelius Riemenschneider Date: Thu, 22 Aug 2024 10:54:09 +0200 Subject: [PATCH 2/2] Rename grouping information to be decoupled from loadgroup, and provide it to all schedulers. --- src/xdist/dsession.py | 4 ++-- src/xdist/remote.py | 6 +++--- src/xdist/scheduler/each.py | 5 +++++ src/xdist/scheduler/load.py | 4 ++++ src/xdist/scheduler/loadgroup.py | 34 ++++---------------------------- src/xdist/scheduler/loadscope.py | 4 ++++ src/xdist/scheduler/protocol.py | 2 ++ src/xdist/scheduler/worksteal.py | 4 ++++ src/xdist/workermanage.py | 2 +- 9 files changed, 29 insertions(+), 36 deletions(-) diff --git a/src/xdist/dsession.py b/src/xdist/dsession.py index f105dc75..746d80a0 100644 --- a/src/xdist/dsession.py +++ b/src/xdist/dsession.py @@ -275,7 +275,7 @@ def worker_collectionfinish( self, node: WorkerController, ids: Sequence[str], - loadgroup_scopes: dict[str, str], + group_markers: dict[str, str], ) -> None: """Worker has finished test collection. @@ -293,7 +293,7 @@ def worker_collectionfinish( assert self._session is not None self._session.testscollected = len(ids) assert self.sched is not None - self.sched.loadgroup_scopes = loadgroup_scopes + self.sched.set_group_markers(group_markers) self.sched.add_node_collection(node, ids) if self.terminal: self.trdist.setstatus( diff --git a/src/xdist/remote.py b/src/xdist/remote.py index d64661d9..c38434eb 100644 --- a/src/xdist/remote.py +++ b/src/xdist/remote.py @@ -204,7 +204,7 @@ def run_one_test(self) -> None: @pytest.hookimpl def pytest_collection_finish(self, session: pytest.Session) -> None: # collect the scope for each node for --dist=loadgroup - loadgroup_scopes = {} + group_markers = {} for item in session.items: mark = item.get_closest_marker("xdist_group") if not mark: @@ -214,12 +214,12 @@ def pytest_collection_finish(self, session: pytest.Session) -> None: if len(mark.args) > 0 else mark.kwargs.get("name", "default") ) - loadgroup_scopes[item.nodeid] = gname + group_markers[item.nodeid] = gname self.sendevent( "collectionfinish", topdir=str(self.config.rootpath), ids=[item.nodeid for item in session.items], - loadgroup_scopes=loadgroup_scopes, + group_markers=group_markers, ) @pytest.hookimpl diff --git a/src/xdist/scheduler/each.py b/src/xdist/scheduler/each.py index aa4f7ba1..1e896d68 100644 --- a/src/xdist/scheduler/each.py +++ b/src/xdist/scheduler/each.py @@ -31,12 +31,17 @@ def __init__(self, config: pytest.Config, log: Producer | None = None) -> None: self.node2pending: dict[WorkerController, list[int]] = {} self._started: list[WorkerController] = [] self._removed2pending: dict[WorkerController, list[int]] = {} + self.group_markers: dict[str, str] = {} + if log is None: self.log = Producer("eachsched") else: self.log = log.eachsched self.collection_is_completed = False + def set_group_markers(self, group_markers: dict[str, str]) -> None: + self.group_markers = group_markers + @property def nodes(self) -> list[WorkerController]: """A list of all nodes in the scheduler.""" diff --git a/src/xdist/scheduler/load.py b/src/xdist/scheduler/load.py index 9d153bb9..f3b56d87 100644 --- a/src/xdist/scheduler/load.py +++ b/src/xdist/scheduler/load.py @@ -63,6 +63,7 @@ def __init__(self, config: pytest.Config, log: Producer | None = None) -> None: self.node2pending: dict[WorkerController, list[int]] = {} self.pending: list[int] = [] self.collection: list[str] | None = None + self.group_markers: dict[str, str] = {} if log is None: self.log = Producer("loadsched") else: @@ -70,6 +71,9 @@ def __init__(self, config: pytest.Config, log: Producer | None = None) -> None: self.config = config self.maxschedchunk = self.config.getoption("maxschedchunk") + def set_group_markers(self, group_markers: dict[str, str]) -> None: + self.group_markers = group_markers + @property def nodes(self) -> list[WorkerController]: """A list of all nodes in the scheduler.""" diff --git a/src/xdist/scheduler/loadgroup.py b/src/xdist/scheduler/loadgroup.py index 0cc1cf0d..72de4b16 100644 --- a/src/xdist/scheduler/loadgroup.py +++ b/src/xdist/scheduler/loadgroup.py @@ -24,35 +24,9 @@ def __init__(self, config: pytest.Config, log: Producer | None = None) -> None: def _split_scope(self, nodeid: str) -> str: """Determine the scope (grouping) of a nodeid. - There are usually 3 cases for a nodeid:: - - example/loadsuite/test/test_beta.py::test_beta0 - example/loadsuite/test/test_delta.py::Delta1::test_delta0 - example/loadsuite/epsilon/__init__.py::epsilon.epsilon - - #. Function in a test module. - #. Method of a class in a test module. - #. Doctest in a function in a package. - - With loadgroup, two cases are added:: - - example/loadsuite/test/test_beta.py::test_beta0 - example/loadsuite/test/test_delta.py::Delta1::test_delta0 - example/loadsuite/epsilon/__init__.py::epsilon.epsilon - example/loadsuite/test/test_gamma.py::test_beta0@gname - example/loadsuite/test/test_delta.py::Gamma1::test_gamma0@gname - - This function will group tests with the scope determined by splitting the first ``@`` - from the right. That is, test will be grouped in a single work unit when they have - same group name. In the above example, scopes will be:: - - example/loadsuite/test/test_beta.py::test_beta0 - example/loadsuite/test/test_delta.py::Delta1::test_delta0 - example/loadsuite/epsilon/__init__.py::epsilon.epsilon - gname - gname + Either we get a scope from a `xdist_group` mark (and then return that), or we don't do any grouping. """ - if nodeid in self.loadgroup_scopes: - return self.loadgroup_scopes[nodeid] + if nodeid in self.group_markers: + return self.group_markers[nodeid] else: - return super()._split_scope(nodeid) + return nodeid diff --git a/src/xdist/scheduler/loadscope.py b/src/xdist/scheduler/loadscope.py index a4d63b29..c2ac3c6d 100644 --- a/src/xdist/scheduler/loadscope.py +++ b/src/xdist/scheduler/loadscope.py @@ -97,6 +97,7 @@ def __init__(self, config: pytest.Config, log: Producer | None = None) -> None: self.workqueue: OrderedDict[str, dict[str, bool]] = OrderedDict() self.assigned_work: dict[WorkerController, dict[str, dict[str, bool]]] = {} self.registered_collections: dict[WorkerController, list[str]] = {} + self.group_markers: dict[str, str] = {} if log is None: self.log = Producer("loadscopesched") @@ -105,6 +106,9 @@ def __init__(self, config: pytest.Config, log: Producer | None = None) -> None: self.config = config + def set_group_markers(self, group_markers: dict[str, str]) -> None: + self.group_markers = group_markers + @property def nodes(self) -> list[WorkerController]: """A list of all active nodes in the scheduler.""" diff --git a/src/xdist/scheduler/protocol.py b/src/xdist/scheduler/protocol.py index 0435d15b..f579109f 100644 --- a/src/xdist/scheduler/protocol.py +++ b/src/xdist/scheduler/protocol.py @@ -27,6 +27,8 @@ def add_node_collection( collection: Sequence[str], ) -> None: ... + def set_group_markers(self, group_markers: dict[str, str]) -> None: ... + def mark_test_complete( self, node: WorkerController, diff --git a/src/xdist/scheduler/worksteal.py b/src/xdist/scheduler/worksteal.py index fd208486..ad566cc9 100644 --- a/src/xdist/scheduler/worksteal.py +++ b/src/xdist/scheduler/worksteal.py @@ -70,6 +70,7 @@ def __init__(self, config: pytest.Config, log: Producer | None = None) -> None: self.node2pending: dict[WorkerController, list[int]] = {} self.pending: list[int] = [] self.collection: list[str] | None = None + self.group_markers: dict[str, str] = {} if log is None: self.log = Producer("workstealsched") else: @@ -77,6 +78,9 @@ def __init__(self, config: pytest.Config, log: Producer | None = None) -> None: self.config = config self.steal_requested_from_node: WorkerController | None = None + def set_group_markers(self, group_markers: dict[str, str]) -> None: + self.group_markers = group_markers + @property def nodes(self) -> list[WorkerController]: """A list of all nodes in the scheduler.""" diff --git a/src/xdist/workermanage.py b/src/xdist/workermanage.py index cec1f039..f05c34a5 100644 --- a/src/xdist/workermanage.py +++ b/src/xdist/workermanage.py @@ -422,7 +422,7 @@ def process_from_remote( eventname, node=self, ids=kwargs["ids"], - loadgroup_scopes=kwargs["loadgroup_scopes"], + group_markers=kwargs["group_markers"], ) elif eventname == "runtest_protocol_complete": self.notify_inproc(eventname, node=self, **kwargs)