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

[wip] Fix ignore_path condition #2475

Merged
merged 2 commits into from Jul 3, 2017
Merged

[wip] Fix ignore_path condition #2475

merged 2 commits into from Jul 3, 2017

Conversation

@ant31
Copy link
Contributor

@ant31 ant31 commented Jun 6, 2017

For some legacy reason, I have a container copying source to '/' directly.
From there rootdir='/' and paths gathered by pytest have 3 leading /.

the condition https://github.com/pytest-dev/pytest/pull/2475/files#diff-5b415004cf343fc3c36aebb8aad93882L178
is checking the following and it doesn't match:

if "///endpoints/test/test_api.py" in [local('/endpoints/test')]

This pr simply format the path the way the excludepath is formatted


Thanks for submitting a PR, your contribution is really appreciated!

Here's a quick checklist that should be present in PRs:

  • Add a new news fragment into the changelog folder
    • name it $issue_id.$type for example (588.bug)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of removal, feature, bugfix, vendor, doc or trivial
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."
  • Target: for bugfix, vendor, doc or trivial fixes, target master; for removals or features target features;
  • Make sure to include reasonable tests for your change if necessary

Unless your change is a trivial or a documentation fix (e.g., a typo or reword of a small section) please:

  • Add yourself to AUTHORS;
@ant31 ant31 changed the title Fix ignore_path with rootdir '/' [wip] Fix ignore_path with rootdir '/' Jun 6, 2017
@ant31
Copy link
Contributor Author

@ant31 ant31 commented Jun 6, 2017

Hi,
I am not sure where to add the tests for this, I would appreciate a pointer ?

@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Jun 6, 2017

Thanks for the PR.

Could you please explain the issue this solves in detail? It is not entirely obvious at first glance...

@ant31
Copy link
Contributor Author

@ant31 ant31 commented Jun 6, 2017

@nicoddemus Sorry, I was doing it, (see PR description). Let me know if you need more details

@ant31 ant31 changed the title [wip] Fix ignore_path with rootdir '/' [wip] Fix ignore_path condition Jun 6, 2017
@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Jun 6, 2017

Thanks @ant31 that's clear now

@@ -175,7 +175,7 @@ def pytest_ignore_collect(path, config):
if excludeopt:
ignore_paths.extend([py.path.local(x) for x in excludeopt])

if path in ignore_paths:
if py.path.local(path) in ignore_paths:

This comment has been minimized.

@nicoddemus

nicoddemus Jun 6, 2017
Member

Strange, line 171 is p = path.dirpath(), which suggests path is already a py.path.local instance...

This comment has been minimized.

@nicoddemus

nicoddemus Jun 24, 2017
Member

Gentle ping to @ant31. 😁

I'm asking because I would like to fully understand the patch before it being accepted.

This comment has been minimized.

@ant31

ant31 Jun 24, 2017
Author Contributor

p is a path.dirpath(), but not path.
I removed the local variable p to make it clearer (https://github.com/pytest-dev/pytest/pull/2475/files#diff-5b415004cf343fc3c36aebb8aad93882R171)

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Jun 24, 2017
Member

But the return value of dirpath should be a path ?!?

This comment has been minimized.

@ant31

ant31 Jun 24, 2017
Author Contributor

@RonnyPfannschmidt / @nicoddemus
You are right, it's already a py.path.local

It still fixes my issue because applying twice (?) it strips the leading /:

(Pdb) path
local('///digest')
(Pdb) py.path.local(path)
local('/digest')

not sure where to fix that now

This comment has been minimized.

@nicoddemus

nicoddemus Jun 24, 2017
Member

We can do both to be honest, because it is simple to workaround it here for users which don't have the same py version.

This comment has been minimized.

@nicoddemus

nicoddemus Jul 3, 2017
Member

pytest-dev/py#128 has been merged, should we merge this one as well @RonnyPfannschmidt?

This comment has been minimized.

@nicoddemus

nicoddemus Jul 3, 2017
Member

Oh I just realized we probably need a newsfragment for this as well.

This comment has been minimized.

@RonnyPfannschmidt

RonnyPfannschmidt Jul 3, 2017
Member

@nicoddemus we should merge this one as well after a news fragment is made

This comment has been minimized.

@nicoddemus

nicoddemus Jul 3, 2017
Member

Added a news fragment file. 👍

@coveralls
Copy link

@coveralls coveralls commented Jun 6, 2017

Coverage Status

Coverage remained the same at 92.133% when pulling a9f0f6f on ant31:master into 57e2ced on pytest-dev:master.

@ant31 ant31 force-pushed the ant31:master branch from a9f0f6f to 16df4da Jun 24, 2017
@coveralls
Copy link

@coveralls coveralls commented Jun 24, 2017

Coverage Status

Coverage decreased (-0.0009%) to 92.138% when pulling 16df4da on ant31:master into 6e2b5a3 on pytest-dev:master.

@coveralls
Copy link

@coveralls coveralls commented Jul 3, 2017

Coverage Status

Coverage decreased (-0.0009%) to 92.138% when pulling c578418 on ant31:master into 6e2b5a3 on pytest-dev:master.

@nicoddemus nicoddemus merged commit 6908d93 into pytest-dev:master Jul 3, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Jul 3, 2017

Thanks @ant31!

@ant31
Copy link
Contributor Author

@ant31 ant31 commented Jul 4, 2017

@nicoddemus @RonnyPfannschmidt Awesome, appreciate how fast you fixed it.

(btw, great job on pytest, we're loving it)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants