Skip to content

Fix passing --basetemp to subprocesses with pytest 5.4 #517

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Fix passing --basetemp to subprocesses with pytest 5.4 #517

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5bc8eb0
Fix passing `--basetemp` to subprocesses with pytest 5.4
blueyed Mar 31, 2020
845a524
more tests
blueyed Mar 31, 2020
2ac71e5
extend test_basetemp_in_subprocesses
blueyed Mar 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/510.bugfix.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix passing `--basetemp` to subprocesses with pytest 5.4.
10 changes: 6 additions & 4 deletions src/xdist/workermanage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
from __future__ import print_function

import fnmatch
import os
import re
import sys

import execnet
import py
import pytest
import execnet
from _pytest.tmpdir import TempPathFactory

import xdist.remote

Expand Down Expand Up @@ -252,9 +254,9 @@ def setup(self):
args = make_reltoroot(self.nodemanager.roots, args)
if spec.popen:
name = "popen-%s" % self.gateway.id
if hasattr(self.config, "_tmpdirhandler"):
basetemp = self.config._tmpdirhandler.getbasetemp()
option_dict["basetemp"] = str(basetemp.join(name))
basetemp = TempPathFactory.from_config(self.config).getbasetemp()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does this handle missing basetmp parameters - at first glance i would guess, that each process gets its own basetemp if done this way,

in addition the tempoary folder of the workers would be distinct from the directory of the managing process

for sanity this needs at least share the same root folder

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean, but I've added more tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The standard mode of operation as users observe does not pass the basetmp valve

As such it seems that und normal operation workers will not share a common basetmp unless you hoist the basetmp variable out of the loop

And there will still be a number of potential issues wrt creating tmpdir in the main process

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous self.config._tmpdirhandler should be the same as TempPathFactory.from_config(self.config) (except for that it uses the newer API), no?

The only change / improvement is that it now also works with -p no:tmpdir (a corner case), that previously would silently not use it / skip it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@blueyed you create a new instance each loop, and each instance will make a own numbered tmpdir at the getbasetemp time ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what "loop" you are referring to - I've added one more explicit test, which is the same with xdist master and pytest 5.3.x.

option_dict["basetemp"] = str(basetemp.joinpath(name))

self.config.hook.pytest_configure_node(node=self)

remote_module = self.config.hook.pytest_xdist_getremotemodule()
Expand Down
46 changes: 34 additions & 12 deletions testing/acceptance_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,15 +118,31 @@ def test_fail2():
def test_basetemp_in_subprocesses(self, testdir):
p1 = testdir.makepyfile(
"""
def test_send(tmpdir):
import py
assert tmpdir.relto(py.path.local(%r)), tmpdir
"""
% str(testdir.tmpdir)
import py

def test_send1(tmpdir):
assert tmpdir.relto(py.path.local({!r})), tmpdir
assert str(tmpdir) == {!r}

def test_send2(tmpdir):
assert tmpdir.relto(py.path.local({!r})), tmpdir
assert str(tmpdir) == {!r}
""".format(
str(testdir.tmpdir),
str(
testdir.tmpdir.dirpath()
/ "test_basetemp_in_subprocesses0/runpytest-0/popen-gw0/test_send10"
),
str(testdir.tmpdir),
str(
testdir.tmpdir.dirpath()
/ "test_basetemp_in_subprocesses0/runpytest-0/popen-gw1/test_send20"
),
)
)
result = testdir.runpytest_subprocess(p1, "-n1")
result = testdir.runpytest_subprocess(p1, "-n2")
assert result.ret == 0
result.stdout.fnmatch_lines(["*1 passed*"])
result.stdout.fnmatch_lines(["* 2 passed in *"])

def test_dist_ini_specified(self, testdir):
p1 = testdir.makepyfile(
Expand Down Expand Up @@ -766,13 +782,19 @@ def test_tmpdir_disabled(testdir):
"""
p1 = testdir.makepyfile(
"""
def test_ok():
pass
"""
def test_ok1(request):
assert request.config.option.basetemp == {!r}

def test_ok2(request):
assert request.config.option.basetemp == {!r}
""".format(
str(testdir.tmpdir.dirpath() / "basetemp" / "popen-gw0"),
str(testdir.tmpdir.dirpath() / "basetemp" / "popen-gw1"),
)
)
result = testdir.runpytest(p1, "-n1", "-p", "no:tmpdir")
result = testdir.runpytest(p1, "-n2", "-p", "no:tmpdir")
assert result.ret == 0
result.stdout.fnmatch_lines("*1 passed*")
result.stdout.fnmatch_lines("* 2 passed in *")


@pytest.mark.parametrize("plugin", ["xdist.looponfail", "xdist.boxed"])
Expand Down