Retain input ordering in loadscope (Rebase code for #1098)
#1217
Conversation
* Optionally retain input ordering in loadscope for tests where relative ordering matters. i.e. guarantee that, given [input_1, input_2], input_2 never runs before input_1. On any given worker, either input_ has ran before input_2, or input_1 has never and will never run on this worker.
for more information, see https://pre-commit.ci
darwintree
changed the title
loadscope (Rebase code for #1098)
src/xdist/plugin.py
Outdated
| @@ -127,6 +127,17 @@ def pytest_addoption(parser: pytest.Parser) -> None: | |||
| "(default) no: Run tests inprocess, don't distribute." | |||
| ), | |||
| ) | |||
| group.addoption( | |||
| "--no-loadscope-reorder", | |||
There was a problem hiding this comment.
If particularly find negative options like this confusing. How about we change it to --loadscope-reorder and default it to True?
There was a problem hiding this comment.
I think the original specification might be more canonical.
I found some discussions on the flag name: https://stackoverflow.com/a/15008806/14223776.
However the dest variable naming could be improved. Will modify later
There was a problem hiding this comment.
--load-scope and --no-load-scope seems good to me. 👍
There was a problem hiding this comment.
--load-scopeand--no-load-scopeseems good to me. 👍
--load-scope might be confusing. As this feature is only enabled on --dist loadscope and it just affects the loadscope order behaviour. if --loadscope-reorder would be ok, I would add another option --loadscope-reorder (whereas, --loadscope-reorder and --no-loadscope-reorder will be finally added)
There was a problem hiding this comment.
whereas, --loadscope-reorder and --no-loadscope-reorder will be finally added
Ahh sorry, yes, that is what I meant actually. 👍
|
Im not quite sure how to take this I haven't been able to review the original pr which is part of why the original pr hasn't been rebased I won't magically be able to review a rebase |
|
@RonnyPfannschmidt, that's a fair point. From my perspective, the PR is relatively small. Do you have any concerns about the functionality or implementation that make you want to take more time to evaluate it further? |
|
Im perhaps paranoid about order details The code is dealing with the wrong id types |
Would you specify what does "wrong id types" mean? Would be glad to resolve the issue |
|
@RonnyPfannschmidt can you clarify your concerns? The patch as is seems really straightforward to me. Or do you have concerns about the original code? Even if that is the case, I don't think we need to block this patch because of it. |
|
@RonnyPfannschmidt gentle ping. 👍 |
|
I'll defer judgement and leave it to the others |
This is a pull request coming from #1098 to enable the possibility to merge.
This pull request rebase the original changes to the latest master branch commit.
I'm not the original author but would be willing to help improve the code if there is any comments
Closes #1098
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: