pytest-dev / pytest-xdist

pytest plugin for distributed testing and loop-on-failures testing modes.
https://pytest-xdist.readthedocs.io
MIT License
1.49k stars 232 forks source link

Loadgroup: Don't modify test nodeids any longer. #1119

Open criemen opened 3 months ago

criemen commented 3 months ago

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.

I'm not so keen on the "protocol" between the test hook and the schedulers, as we're currently just writing into a field of the currently active scheduler with this implementation. I previously considered adding this as an extra argument to add_node_collection, but that then requires touching all schedulers for something that's only relevant to a single one. Other ideas are welcome here in code review!

This is motivated by https://github.com/pytest-dev/pytest-xdist/pull/1118.

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

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

criemen commented 3 months ago

I'll fix the mypy complaints once we've decided on a viable approach for passing the info into the scheduler. Changelog is coming then, too.

criemen commented 3 months ago

It's maybe worth saying that I first wanted to put the scopes from the test collection into config.stash, but that's not possible, as there's the execnet indirection between the process where the pytest test collection runs on (presumably a "worker"), and the actual process that runs the test loop. That's been a bit frustrating, and as someone who's just using -nauto, that's a feature that's complicating the implementation of xdist needlessly.

RonnyPfannschmidt commented 3 months ago

A communication channel for those details has to be created, it's time consuming and coordination heavy

Hence the lack of a implementation of that so far

criemen commented 3 months ago

I just pushed a change that renames the parameter. I thought that we can make this information available to all schedulers, and we could even change the whole load* family to use the grouping information by default, and deprecate theloadgroup strategy. I'd also love to respect the grouping in worksteal, but that looks a little bit trickier to implement.