-
Notifications
You must be signed in to change notification settings - Fork 234
Use Config.invocation_params for consistent worker initialization #448
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
Initialization between workers and master nodes is now more consistent, which fixes a number of | ||
long-standing issues related to startup with the ``-c`` option. | ||
|
||
Issues: | ||
|
||
* `#6 <https://github.com/pytest-dev/pytest-xdist/issues/6>`__: Poor interaction between ``-n#`` and ``-c X.cfg`` | ||
* `#445 <https://github.com/pytest-dev/pytest-xdist/issues/445>`__: pytest-xdist is not reporting the same nodeid as pytest does | ||
|
||
This however only works with **pytest 5.1 or later**, as it required changes in pytest itself. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -186,6 +186,8 @@ def make_reltoroot(roots, args): | |
for arg in args: | ||
parts = arg.split(splitcode) | ||
fspath = py.path.local(parts[0]) | ||
if not fspath.exists(): | ||
continue | ||
for root in roots: | ||
x = fspath.relto(root) | ||
if x or fspath == root: | ||
|
@@ -236,10 +238,14 @@ def shutting_down(self): | |
def setup(self): | ||
self.log("setting up worker session") | ||
spec = self.gateway.spec | ||
args = self.config.args | ||
if hasattr(self.config, "invocation_params"): | ||
args = [str(x) for x in self.config.invocation_params.args or ()] | ||
option_dict = {} | ||
else: | ||
args = self.config.args | ||
option_dict = vars(self.config.option) | ||
if not spec.popen or spec.chdir: | ||
args = make_reltoroot(self.nodemanager.roots, args) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nicoddemus I think now I correctly identified the cause of the problem. In the new code the args stripping matters cause the old option_dict is no more, while previously this stripping had no effect. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, what "stripping" you mean? Can you please clarify? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nicoddemus see #491 - make_reltoroot removes anything that ain't a valid path (like the arguments I need). |
||
option_dict = vars(self.config.option) | ||
if spec.popen: | ||
name = "popen-%s" % self.gateway.id | ||
if hasattr(self.config, "_tmpdirhandler"): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this give unwanted arguments to the worker?
I'm investigating why pytest-cov's test suite is broken on 0.30 and invocation_params.args has lots of stuff while config.args is just the test file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicoddemus
Can you comment, please?
(I'm currently looking into getting pytest-cov's CI to pass again so this would help there)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @ionelmc and @blueyed, I must have missed this.
Why do you say it would give unwanted arguments to the worker? Using
invocation_params
it will initialize exactly like the master process (well almost), which solved a problem with initialization of the config file.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicoddemus I guess I'm just confused about what's wrong with the latest pytest/xdist/cov combination (it's broken in some situations). Any explanation about how the all works would help me debug it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure myself, but the latest implementation just forwards all the parameters given in the command line to the workers, making them initialize the same way as the master node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicoddemus the way I understand this the problem is that option_dict is empty in the new "invocation_params mode", thus plugins like pytest-cov don't have the necessary options to initialize. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless pytest-cov is trying to get the options from pytest-xdist structures instead of
config.getoption
, it should just work. The workers should now be initializing the same way as the master node/without xdist, so all the options should be there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicoddemus the problem is that none of the plugin options are there when stuff like
--dist=load --tx=popen//chdir=1
is used. When using-n 1
everything is fine.