-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Paths after normal options are now properly used to discover rootdir and ini files #961
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
Conversation
|
||
@pytest.mark.parametrize('args', [ | ||
['dir1', 'dir2', '-v'], | ||
['dir1', '-v', 'dir2'], |
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.
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
).
looks good to me, but i'd like someone else to review as well |
elif arg == 'dir2': | ||
args[i] = d2 | ||
result = testdir.runpytest(*args) | ||
result.stdout.fnmatch_lines(['*rootdir: *myroot, inifile: ']) |
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.
please assert which root is actually set, not only that it's found at all
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.
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?
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.
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?
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.
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?
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.
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 [email protected]
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.
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.
I think @RonnyPfannschmidt said the patch seemed OK, so feel free to merge. 😄
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.
Went ahead and did it myself, as I noticed you replied by email and might be on the run.
Thanks!
one comment |
Paths after normal options are now properly used to discover rootdir and ini files
Fix #949