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

conftest.py fixes #840

Merged
merged 5 commits into from
Jul 9, 2018
Merged

conftest.py fixes #840

merged 5 commits into from
Jul 9, 2018

Conversation

notestaff
Copy link
Contributor

Changed tmpdir_{session,module,function} to use tempfile.mkdtemp(), and to check if the tempdir still exists before trying to erase it.
In tmpdir_function() switch to using monkeypatch; this fixes a bug
where if TMPDIR was originally unset it will be left set to the
now-deleted tempdir.

and to check if the tempdir still exists before trying to erase it.
In tmpdir_function() switch to using monkeypatch; this fixes a bug
where if TMPDIR was originally unset it will be left set to the
now-deleted tempdir.
@notestaff notestaff self-assigned this Jul 6, 2018
Copy link
Member

@dpark01 dpark01 left a comment

Choose a reason for hiding this comment

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

To the extent that I can see, this overall looks okay to me but I think I'd feel more comfortable if someone who was more familiar with py.test fixtures took a look at this too. For example, turning these all into iterators is okay, right? (I don't know)

conftest.py Outdated
@@ -29,46 +31,44 @@ def pytest_configure(config):
reporter = FixtureReporter(config)
config.pluginmanager.register(reporter, 'fixturereporter')

def _max_fname_len(file_system_path, default_max_len=80):
name_max_str = [s for s in os.pathconf_names if s.endswith('_NAME_MAX')]
Copy link
Member

Choose a reason for hiding this comment

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

_PATH_MAX?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is for a (prefix of) a path component, not full path

Copy link
Contributor Author

Choose a reason for hiding this comment

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

turning into iterators is for specifying cleanup actions after fixture goes out of scope

conftest.py Outdated
pass
return default_max_len

def _make_fname_valid(file_system_path, fname, len_margin):
Copy link
Member

Choose a reason for hiding this comment

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

What's invalid about non-ascii filenames? Is there any reason this needs to be enforced?

Copy link
Member

Choose a reason for hiding this comment

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

(if it does need to be enforced, we have util.file.string_to_file_name())

Copy link
Member

Choose a reason for hiding this comment

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

Actually the commented out portions of that function show that we made a point of allowing unicode filenames, we just disallowed certain pathsep or escape characters.

conftest.py Outdated
name = _make_fname_valid(file_system_path=basetemp, fname=name, len_margin=50)
tmpdir = tempfile.mkdtemp(dir=basetemp, prefix='test-{}-{}-'.format(scope, name))
yield tmpdir
if os.path.isdir(tmpdir): shutil.rmtree(tmpdir)
Copy link
Member

Choose a reason for hiding this comment

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

Don't we still think we should shutil.rmtree(ignore_errors=True)? Our error before in #839 was not about the tmpdir not existing, it was about a file within the tmpdir disappearing while rmtree was in the middle of walking the tree.

And if we ignore_errors=True then the if os.path.isdir becomes unnecessary as a side effect.

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.

3 participants