Loadgroup: Don't modify test nodeids any longer. #1119
Conversation
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'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. |
|
It's maybe worth saying that I first wanted to put the scopes from the test collection into |
|
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 |
There was a problem hiding this comment.
I like this, instead of a full communication channel that includes markers and other details
We pass a dedicated value to enable this usecase
Let's iterate a bit on the names passed to ensure intent is clear without the reader knowing All the details
In future we want to pass richer details on the collected tests so scheduled can use fixture scopes, parameters and active fixtures as well for decision, but that needs the previously mentioned Design proces
Your solution nicely contains the hack
src/xdist/remote.py
Outdated
| if len(mark.args) > 0 | ||
| else mark.kwargs.get("name", "default") | ||
| ) | ||
| loadgroup_scopes[item.nodeid] = gname |
There was a problem hiding this comment.
An important Note here is that unfortunately duplicate node ids are only discouraged,not enforced
Which is the reason for using the node index instead of the node id when communicating using the scheduler
There was a problem hiding this comment.
That's pretty bad. What's your opinion, does this need to be addressed, or can we live with duplicate node ids being scheduled on the same node, per xdist_group, even only one test had the annotation?
and provide it to all schedulers.
|
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 |
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 #1118.
Thanks for submitting a PR, your contribution is really appreciated!
Here's a quick checklist that should be present in PRs:
Make sure to include reasonable tests for your change if necessary
We use towncrier for changelog management, so please add a news file into the
changelogfolder following these guidelines:Name it
$issue_id.$typefor example588.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,docortrivialMake sure to use full sentences with correct case and punctuation, for example: