Skip to content
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

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

Closed
wants to merge 3 commits into from

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Mar 31, 2020

Fixes #510.
Ref: pytest-dev/pytest#6767

Also uses --basetemp with -p no:tmpdir as a good side effect
(ref: #22).

Current failure with py27 due to 4.6.x missing pytest-dev/pytest@3e669a262 (pytest-dev/pytest#6870).

Fixes pytest-dev#510.
Ref: pytest-dev/pytest#6767

Also uses `--basetemp` with `-p no:tmpdir` as a good side effect
(ref: pytest-dev#22).
@blueyed
Copy link
Contributor Author

blueyed commented Mar 31, 2020

Failures with pytestmaster are unrelated (due to pytest-dev/pytest#6981).

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.

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

with this i currently get

$ rm -rf show
$ mkdir show
$ tree show
show

0 directories, 0 files
$ TMPDIR=./show pytest -n3 testing/test_slavemanage.py 
============================================================================================================ test session starts =============================================================================================================
platform linux -- Python 3.7.6, pytest-5.4.1, py-1.8.1, pluggy-0.13.1
rootdir: /home/rpfannsc/Projects/pytest-dev/pytest-xdist, inifile: tox.ini
plugins: forked-1.1.3, xdist-1.31.1.dev14+g9bc2a95
gw0 [14] / gw1 [14] / gw2 [14]
........xs....                                                                                                                                                                                                                         [100%]
========================================================================================================== short test summary info ===========================================================================================================
SKIPPED [1] /home/rpfannsc/Projects/pytest-dev/pytest-xdist/.env/lib/python3.7/site-packages/_pytest/config/__init__.py:1165: no 'gspecs' option found
XFAIL testing/test_slavemanage.py::TestNodeManager::test_rsync_roots_no_roots
  reason: [NOTRUN] 
================================================================================================== 12 passed, 1 skipped, 1 xfailed in 0.58s ==================================================================================================
$ tree -L 3 show 
show
└── pytest-of-rpfannsc
    ├── pytest-0
    │   └── popen-gw0
    ├── pytest-1
    │   └── popen-gw1
    ├── pytest-2
    │   └── popen-gw2
    └── pytest-current -> /home/rpfannsc/Projects/pytest-dev/pytest-xdist/show/pytest-of-rpfannsc/pytest-2

8 directories, 0 files

@blueyed
Copy link
Contributor Author

blueyed commented Apr 1, 2020

Ok, not worth/easy fixing it in xdist then probably: it could use a single basetemp itself (e.g. via pytest_session_start), but let's keep it in pytest then.

@blueyed blueyed closed this Apr 1, 2020
@blueyed blueyed deleted the fix-basetemp branch April 1, 2020 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test failure with pytest master
2 participants