Custom test grouping and test group order logic #500

SalmonMode · 2020-01-19T18:01:05Z

Closes #18

It's been long requested (#18) to allow for custom logic for the distribution of tests among test groups in a simple and clean way. This adds that functionality by providing a hook that each tests nodeid is passed to, and whatever is returned by the hook for a given test is that test's test group name. All tests with the same test group name will be batched together and passed to the same worker together.

I also added another hook that grants access to the finished collection of test groups so that the order of the groups can be changed as needed. This allows for certain optimizations to be made, such as putting slow tests in the same group and placing that group in the beginning of the collection in order to speed the test suite up.

I also took the opportunity to clean up the logic quite a bit as there was a lot of repetition between the different load schedulers. Now they are unified into the same base class (except for EachScheduling) with the only differing logic being what their default grouping logic is. I felt I should leave EachScheduling alone for now, but I imagine it could also use the same base class by simply having every test in the same group, and then duplicating that group N times. But that seemed kinda memory heavy.

Thanks for submitting a PR, your contribution is really appreciated!

Here's a quick checklist that should be present in PRs:

[x ] Make sure to include reasonable tests for your change if necessary
[x ] We use towncrier for changelog management, so please add a news file into the changelog folder following these guidelines:
- Name it $issue_id.$type for example 588.bugfix;
- If you don't have an issue_id change it to the PR id after creating it
- Ensure type is one of removal, feature, bugfix, vendor, doc or trivial
- Make sure to use full sentences with correct case and punctuation, for example:
```
Fix issue with non-ascii contents in doctest text files.
```

…ordering test groups for execution

nicoddemus · 2020-01-24T22:08:48Z

Hi @SalmonMode,

Thanks a lot for the PR! As you mention this is a long-requested feature so all the work you have put into it is more than welcome!

I like the approach of providing "low level" hooks that lets users and plugins customize test grouping to be sent to workers. Also so far in the review I'm very satisfied with the code quality and documentation you've put into it. 👍

I'm writing this just so to let you know that it will probably take awhile to review a PR of this magnitude, so please be patient! 😁

SalmonMode · 2020-01-24T22:41:17Z

@nicoddemus haha I appreciate it 😁 . Take your time. I spent a very long time on it (granted, at least half of that time was on what to name the hooks).

As a side note, the name I chose was because I figure at some point in the future, this may be expanded to allow grouping based on markers, rather than just names, but I couldn't figure out how to pull those in in a good way, and wanted to get the PR in rather than holding onto it for longer.

ssbarnea · 2020-08-31T18:02:37Z

Any chance to refresh this one?

nicoddemus · 2020-08-31T18:59:41Z

Any chance to refresh this one?

Thanks for the ping @ssbarnea! Indeed I had lost sight of this.

Will make time for completing my review this week. 👍

skhomuti · 2021-01-12T09:02:20Z

@nicoddemus just reminding about the existence of this PR :)

Wilsontomass · 2022-08-29T15:26:00Z

Heya @nicoddemus, sorry for the bump but we would be extremely grateful for a PR of this kind to be added to pytest-xdist. Is it still feasable? Has some other functionality superseded it?

RonnyPfannschmidt

This is quite large, so itll be a while before I can take a deeper look

I want to note that it's unfortunate we don't have a good way to transfer details about marks with the nodeids, we should add a hook that allows to inform the coordinator about affinities (session scope fixtures, certain configurations)

nicoddemus · 2022-08-30T12:51:25Z

Thanks a lot for the ping!

I had left a bunch of comments, but forgot to publish them 🤦 :

I just published them, I will give it another review later.

nicoddemus · 2020-01-24T21:41:16Z

README.rst

+
+By default, there is no grouping logic and every individual test is placed in its own
+test group, so using the ``-n`` option will send pending tests to any worker that is
+available, without any guaranteed order. It should be assumed that when using this


This is a bit misleading in the sense that it suggests every test will run in isolation with their own copies of every fixture (including high-scoped fixtures like session), which is not true: it is just that each worker is its own "session", so high-scoped fixtures will live in that session as if in an isolated pytest executing.

We could rewrite that part, but just removing it altogether is an option too. What do you think?

nicoddemus · 2020-01-24T21:49:37Z

README.rst

+grouping options, based on simple criteria about a test's nodeid. so you can gunarantee
+that certain tests are run in the same process. When they're run in the same process,
+you gunarantee that larger-scoped fixtures are only executed as many times as would
+normally be expected for the tests in the test group. But, once that test group is


The phrase "But, once that test group..." suggests that xdist might be do something special in the sense to destroy the fixtures...

Perhaps we should have a separate section explaining how fixture execution in general works in xdist: each worker is its own session, so high-scope fixtures are bound to that worker, etc. This section applies to xdist in general and is not specific to the test grouping feature.

Back to the docs at hand, we can then just discuss how tests are grouped/sent to workers, without getting into details again regarding fixture setup/teardown.

What do you think? Hope my comments make sense. 😁

nicoddemus · 2020-01-24T21:50:30Z

README.rst

+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+By default, ``pytest-xdist`` doesn't group any tests together, but it provides some
+grouping options, based on simple criteria about a test's nodeid. so you can gunarantee


Suggested change

grouping options, based on simple criteria about a test's nodeid. so you can gunarantee

grouping options, based on simple criteria about a test's nodeid, so you can guarantee

nicoddemus · 2020-01-24T21:50:53Z

README.rst

+groups, as it creates a new groups as needed. You can tap into this system to define
+your own grouping logic by using the ``pytest_xdist_set_test_group_from_nodeid``.
+
+If you define your own copy of that hook, it will be called once for every test, and the


Suggested change

If you define your own copy of that hook, it will be called once for every test, and the

If you define your own implementation of that hook, it will be called once for every test, and the

nicoddemus · 2020-01-24T21:58:21Z

src/xdist/scheduler/load.py

+       ::
+
+            workqueue = {
+                '<full>/<path>/<to>/test_module.py': {


Suggested change

'<full>/<path>/<to>/test_module.py': {

'<group id 1>': {

Perhaps use <group id 1> to illustrate the idea that a group id is anything, not necessarily a filename?

nicoddemus · 2020-01-24T22:00:10Z

src/xdist/scheduler/load.py

+       should always be identical. This is an alias for
+       `.registered_collections``.
+
+    :node2pending: Map of nodes and the names of their pending test groups. The


Probably best to move this to the properties' docstring?

nicoddemus · 2020-01-24T22:00:53Z

src/xdist/scheduler/load.py


    @property
    def collection_is_completed(self):
-        """Boolean indication initial test collection is complete.
+        """Booleanq indication initial test collection is complete.


Suggested change

"""Booleanq indication initial test collection is complete.

"""Boolean indication initial test collection is complete.

nicoddemus · 2022-08-30T12:53:58Z

we should add a hook that allows to inform the coordinator about affinities (session scope fixtures, certain configurations)

Just so we are in the same page, you mean in this PR or in a follow up?

nicoddemus

Reviewed the implementation and it looks good, left a few more comments.

I personally would like to have some of the current code that manipulates workloads/test groups abstracted away into separate classes, instead of having all the details implemented into the scope itself, but definitely this can be done later if someone decides to tackle it.

nicoddemus · 2022-08-31T12:14:34Z

src/xdist/newhooks.py

+def pytest_xdist_order_test_groups(workqueue):
+    """Sort the queue of test groups to determine the order they will be executed in.
+
+    The ``workqueue`` is an ``OrderedDict`` containing all of the test groups in the


Suggested change

The ``workqueue`` is an ``OrderedDict`` containing all of the test groups in the

The ``workqueue`` is an ``OrderedDict`` of ``group => list of node ids`` containing all of the test groups in the

nicoddemus · 2022-08-31T12:16:09Z

src/xdist/scheduler/load.py

+    single test. This is designed to be extensible so that custom grouping
+    logic can be applied either by making a child class from this and
+    overriding the ``get_default_test_group`` method, or by defining the
+    ``pytest_xdist_set_test_group_from_nodeid``.hook. If the hook is used, but it returns


Suggested change

``pytest_xdist_set_test_group_from_nodeid``.hook. If the hook is used, but it returns

``pytest_xdist_set_test_group_from_nodeid`` hook. If the hook is used, but it returns

nicoddemus · 2022-08-31T12:18:32Z

src/xdist/scheduler/load.py

+       ::
+
+            assigned_work = {
+                '<worker node A>': {


Suggested change

'<worker node A>': {

'gw0': {

Using ids as we have them internally will make this easier to understand I think.

nicoddemus · 2022-08-31T12:19:00Z

src/xdist/scheduler/load.py

+       ::
+
+            registered_collections = {
+                '<worker node A>': [


Suggested change

'<worker node A>': [

'gw0': [

nicoddemus · 2022-08-31T12:36:07Z

src/xdist/newhooks.py

@@ -55,3 +55,55 @@ def pytest_xdist_node_collection_finished(node, ids):
 @pytest.mark.firstresult
 def pytest_xdist_make_scheduler(config, log):
    """ return a node scheduler implementation """
+
+
+@pytest.mark.trylast


Suggested change

@pytest.mark.trylast

@pytest.hookspec(trylast=True, firstresult=True)

Besides using the new syntax, I think we should add firstresult here to make it clear in the API that we will only use the first result from the hook.

nicoddemus · 2022-08-31T12:38:51Z

src/xdist/newhooks.py

+
+
+@pytest.mark.trylast
+def pytest_xdist_set_test_group_from_nodeid(nodeid):


Suggested change

def pytest_xdist_set_test_group_from_nodeid(nodeid):

def pytest_xdist_get_test_group_from_nodeid(nodeid):

Perhaps "get" better conveys that we should return the test group? To me "set" conveys that I should set the test group somewhere.

nicoddemus · 2022-08-31T12:44:31Z

src/xdist/scheduler/loadscope.py



-class LoadScopeScheduling(object):
+class LoadScopeScheduling(LoadScheduling):
    """Implement load scheduling across nodes, but grouping test by scope.


So loadfile and loadscope are distribute the tests identically now? Or am I missing something?

Allow defining custom logic for test distribution among groups and re…

c7f5779

…ordering test groups for execution

SalmonMode requested review from nicoddemus and hugovk January 19, 2020 18:01

SalmonMode force-pushed the set-test-group branch from 453908c to c7f5779 Compare January 19, 2020 18:02

fixed comments

16eaf37

nicoddemus mentioned this pull request Feb 12, 2020

pytest-harvest does not work with python-xdist smarie/python-pytest-harvest#32

Closed

RonnyPfannschmidt reviewed Aug 29, 2022

View reviewed changes

nicoddemus reviewed Aug 30, 2022

View reviewed changes

nicoddemus reviewed Aug 31, 2022

View reviewed changes

hugovk removed their request for review November 26, 2022 06:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom test grouping and test group order logic #500

Custom test grouping and test group order logic #500

SalmonMode commented Jan 19, 2020 •

edited

nicoddemus commented Jan 24, 2020

SalmonMode commented Jan 24, 2020

ssbarnea commented Aug 31, 2020

nicoddemus commented Aug 31, 2020

skhomuti commented Jan 12, 2021

Wilsontomass commented Aug 29, 2022

RonnyPfannschmidt left a comment

nicoddemus commented Aug 30, 2022

nicoddemus Jan 24, 2020

nicoddemus Jan 24, 2020

nicoddemus Jan 24, 2020

nicoddemus Jan 24, 2020

nicoddemus Jan 24, 2020

nicoddemus Jan 24, 2020

nicoddemus Jan 24, 2020

nicoddemus commented Aug 30, 2022 •

edited

nicoddemus left a comment •

edited

nicoddemus Aug 31, 2022

nicoddemus Aug 31, 2022

nicoddemus Aug 31, 2022

nicoddemus Aug 31, 2022

nicoddemus Aug 31, 2022

nicoddemus Aug 31, 2022

nicoddemus Aug 31, 2022

	grouping options, based on simple criteria about a test's nodeid. so you can gunarantee
	grouping options, based on simple criteria about a test's nodeid, so you can guarantee

	If you define your own copy of that hook, it will be called once for every test, and the
	If you define your own implementation of that hook, it will be called once for every test, and the

	"""Booleanq indication initial test collection is complete.
	"""Boolean indication initial test collection is complete.

	The ``workqueue`` is an ``OrderedDict`` containing all of the test groups in the
	The ``workqueue`` is an ``OrderedDict`` of ``group => list of node ids`` containing all of the test groups in the

	``pytest_xdist_set_test_group_from_nodeid``.hook. If the hook is used, but it returns
	``pytest_xdist_set_test_group_from_nodeid`` hook. If the hook is used, but it returns

	@pytest.mark.trylast
	@pytest.hookspec(trylast=True, firstresult=True)



		@pytest.mark.trylast
		def pytest_xdist_set_test_group_from_nodeid(nodeid):

	def pytest_xdist_set_test_group_from_nodeid(nodeid):
	def pytest_xdist_get_test_group_from_nodeid(nodeid):

Custom test grouping and test group order logic #500

Are you sure you want to change the base?

Custom test grouping and test group order logic #500

Conversation

SalmonMode commented Jan 19, 2020 • edited

nicoddemus commented Jan 24, 2020

SalmonMode commented Jan 24, 2020

ssbarnea commented Aug 31, 2020

nicoddemus commented Aug 31, 2020

skhomuti commented Jan 12, 2021

Wilsontomass commented Aug 29, 2022

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

nicoddemus commented Aug 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus commented Aug 30, 2022 • edited

nicoddemus left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SalmonMode commented Jan 19, 2020 •

edited

nicoddemus commented Aug 30, 2022 •

edited

nicoddemus left a comment •

edited