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

Paths after normal options are now properly used to discover rootdir and ini files #961

Merged
merged 1 commit into from
Aug 26, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions CHANGELOG
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,10 @@
- issue951: add new record_xml_property fixture, that supports logging
additional information on xml output. Thanks David Diaz for the PR.

- issue949: paths after normal options (for example `-s`, `-v`, etc) are now
properly used to discover `rootdir` and `ini` files.
Thanks Peter Lauri for the report and Bruno Oliveira for the PR.

2.7.3 (compared to 2.7.2)
-----------------------------

Expand Down
10 changes: 6 additions & 4 deletions _pytest/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,7 @@ def parse_setoption(self, args, option):
def parse_known_args(self, args):
optparser = self._getparser()
args = [str(x) for x in args]
return optparser.parse_known_args(args)[0]
return optparser.parse_known_args(args)

def addini(self, name, help, type=None, default=None):
""" register an ini-file option.
Expand Down Expand Up @@ -879,8 +879,9 @@ def pytest_load_initial_conftests(self, early_config):
self.pluginmanager._set_initial_conftests(early_config.known_args_namespace)

def _initini(self, args):
parsed_args = self._parser.parse_known_args(args)
r = determine_setup(parsed_args.inifilename, parsed_args.file_or_dir)
parsed_args, extra_args = self._parser.parse_known_args(args)
r = determine_setup(parsed_args.inifilename,
parsed_args.file_or_dir + extra_args)
self.rootdir, self.inifile, self.inicfg = r
self._parser.extra_info['rootdir'] = self.rootdir
self._parser.extra_info['inifile'] = self.inifile
Expand All @@ -900,7 +901,8 @@ def _preparse(self, args, addopts=True):
except ImportError as e:
self.warn("I2", "could not load setuptools entry import: %s" % (e,))
self.pluginmanager.consider_env()
self.known_args_namespace = ns = self._parser.parse_known_args(args)
ns, _ = self._parser.parse_known_args(args)
self.known_args_namespace = ns
if self.known_args_namespace.confcutdir is None and self.inifile:
confcutdir = py.path.local(self.inifile).dirname
self.known_args_namespace.confcutdir = confcutdir
Expand Down
25 changes: 25 additions & 0 deletions testing/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,31 @@ def test_invalid_options_show_extra_information(testdir):
"* rootdir: %s*" % testdir.tmpdir,
])


@pytest.mark.parametrize('args', [
['dir1', 'dir2', '-v'],
['dir1', '-v', 'dir2'],
Copy link
Member Author

Choose a reason for hiding this comment

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

This case and the next would fail before this patch, with rootdir being set to myroot/dir1 and myroot/dir2 respectively (effectively ignoring the 3rd argument when determining rootdir).

['dir2', '-v', 'dir1'],
['-v', 'dir2', 'dir1'],
])
def test_consider_args_after_options_for_rootdir_and_inifile(testdir, args):
"""
Consider all arguments in the command-line for rootdir and inifile
discovery, even if they happen to occur after an option. #949
"""
# replace "dir1" and "dir2" from "args" into their real directory
root = testdir.tmpdir.mkdir('myroot')
d1 = root.mkdir('dir1')
d2 = root.mkdir('dir2')
for i, arg in enumerate(args):
if arg == 'dir1':
args[i] = d1
elif arg == 'dir2':
args[i] = d2
result = testdir.runpytest(*args)
result.stdout.fnmatch_lines(['*rootdir: *myroot, inifile: '])
Copy link
Member

Choose a reason for hiding this comment

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

please assert which root is actually set, not only that it's found at all

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the feedback!

But what do you mean "is actually set"? If the pytest headers show rootdir: /path/to/folder, doesn't that mean that path/to/folder is already set as rootdir? Or do you mean something else?

Copy link
Member

Choose a reason for hiding this comment

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

exactly, but result.stdout.fnmatch_lines(['*rootdir: *myroot, inifile: ']) only asserts that myroot is choosen, and actually both dir1 and dir2 are subfolders of that root
but what if i put folders which are not children of the same folder?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm I see.

But the point of the test was to make sure if you inserted other options in the middle of the paths, the paths to the right should still be considered to determine the rootdir. What you are asking seems to be to test the normal semantics for rootdir, which is already tested down the file (in the TestRootdir class). Or am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

yep, thanks makes sense then
ok, then im fine with merging, should i do it or Ronny?

On Wed, Aug 26, 2015 at 5:42 PM Bruno Oliveira notifications@github.com
wrote:

In testing/test_config.py
#961 (comment):

+def test_consider_args_after_options_for_rootdir_and_inifile(testdir, args):

  • """
  • Consider all arguments in the command-line for rootdir and inifile
  • discovery, even if they happen to occur after an option. Mixing relative and absolute paths to tests fails pytest.ini lookup #949
  • """
  • replace "dir1" and "dir2" from "args" into their real directory

  • root = testdir.tmpdir.mkdir('myroot')
  • d1 = root.mkdir('dir1')
  • d2 = root.mkdir('dir2')
  • for i, arg in enumerate(args):
  •    if arg == 'dir1':
    
  •        args[i] = d1
    
  •    elif arg == 'dir2':
    
  •        args[i] = d2
    
  • result = testdir.runpytest(*args)
  • result.stdout.fnmatch_lines(['*rootdir: *myroot, inifile: '])

Hmmm I see.

But the point of the test was to make sure if you inserted other options
in the middle of the paths, the paths to the right should still be
considered to determine the rootdir. What you are asking seems to be to
test the normal semantics for rootdir, which is already tested down the
file (in the TestRootdir class). Or am I missing something?


Reply to this email directly or view it on GitHub
https://github.com/pytest-dev/pytest/pull/961/files#r37997786.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think @RonnyPfannschmidt said the patch seemed OK, so feel free to merge. 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Went ahead and did it myself, as I noticed you replied by email and might be on the run.

Thanks!



@pytest.mark.skipif("sys.platform == 'win32'")
def test_toolongargs_issue224(testdir):
result = testdir.runpytest("-m", "hello" * 500)
Expand Down
4 changes: 3 additions & 1 deletion testing/test_parseopt.py
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,10 @@ def test_parse2(self, parser):
def test_parse_known_args(self, parser):
parser.parse_known_args([py.path.local()])
parser.addoption("--hello", action="store_true")
ns = parser.parse_known_args(["x", "--y", "--hello", "this"])
ns, extra_args = parser.parse_known_args(["x", "--y", "--hello", "this"])
assert ns.hello
assert ns.file_or_dir == ['x']
assert extra_args == ['--y', 'this']

def test_parse_will_set_default(self, parser):
parser.addoption("--hello", dest="hello", default="x", action="store")
Expand Down