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

Add "--deselect" command line option #3201

Merged
merged 1 commit into from
Feb 16, 2018

Conversation

uSpike
Copy link
Member

@uSpike uSpike commented Feb 9, 2018

Fixes #3198

I implemented this as a new CLI option --deselect since it does have different functionality than --ignore, which completely ignores the file.

The code matches any nodeid that startswith() any --deselect flag.

  • Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.
  • Target the features branch for new features and removals/deprecations.
  • Include documentation when adding new features.
  • Include new tests or update existing tests when applicable.
  • Add yourself to AUTHORS in alphabetical order

@coveralls
Copy link

coveralls commented Feb 10, 2018

Coverage Status

Coverage increased (+0.01%) to 92.65% when pulling 774c539 on uSpike:deselect_cli into 063e2da on pytest-dev:features.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks @uSpike!

""")
result = testdir.runpytest("--deselect=test_a.py::test_a2[1]")
assert result.ret == 0
result.stdout.fnmatch_lines(["*11 passed, 1 deselected*"])
Copy link
Member

Choose a reason for hiding this comment

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

This should be 10 passed, 1 deselected right?

Copy link
Member

Choose a reason for hiding this comment

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

Also I think it would be better to use -v and deselect two tests and ensure they are not being run. You can parametrize test_a2 using a smaller value (say 3) so the test remains tidy and small.

_pytest/main.py Outdated
for colitem in items:
for opt in deselectopt:
if colitem.nodeid.startswith(opt):
deselected.append(colitem)
Copy link
Member

Choose a reason for hiding this comment

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

I believe this needs a break after deselected.append(colitem)

_pytest/main.py Outdated
deselected = []
for colitem in items:
for opt in deselectopt:
if colitem.nodeid.startswith(opt):
Copy link
Member

Choose a reason for hiding this comment

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

startswith can take a tuple which can be used as a much better selector

instead of a loop and the for else hack it could simply use colitem.nodeid.startswith(deselect_prefixes)

i presume the break was not added due to the expectation of the prefixes being distinct/non-overlapping

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh cool, thanks I didnt realize str.startswith() takes a tuple. That's great.

_pytest/main.py Outdated
@@ -66,6 +66,8 @@ def pytest_addoption(parser):
help="try to interpret all arguments as python packages.")
group.addoption("--ignore", action="append", metavar="path",
help="ignore path during collection (multi-allowed).")
group.addoption("--deselect", action="append", metavar="item",
Copy link
Member

Choose a reason for hiding this comment

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

i propose a metavar of nodeid_prefix

@uSpike uSpike force-pushed the deselect_cli branch 2 times, most recently from d36ada9 to f7828a8 Compare February 15, 2018 22:03
@uSpike
Copy link
Member Author

uSpike commented Feb 15, 2018

@RonnyPfannschmidt @nicoddemus I've implemented your suggestions, thanks much for the feedback.

_pytest/main.py Outdated
remaining = []
deselected = []
for colitem in items:
if colitem.nodeid.startswith(tuple(deselect_prefixes)):
Copy link
Member

Choose a reason for hiding this comment

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

well done, please also move the call to tuple to the place where deselect_prefixes is declared

@uSpike uSpike force-pushed the deselect_cli branch 2 times, most recently from 7386d9c to 0e2522c Compare February 16, 2018 14:15
@blueyed
Copy link
Contributor

blueyed commented Feb 16, 2018

In #3198 you said:

I propose to accept test ids with --ignore:

This would not need a new option then.

btw: I've also thought about this being something negating -k (e.g. -K) - but that would be a different feature (since it uses different logic / evaluation).

@uSpike
Copy link
Member Author

uSpike commented Feb 16, 2018

@blueyed yes I had originally thought that I could add to --ignore but the functionality of --ignore ended up being fundamentally different than what I needed. I will update #3198.

@uSpike uSpike mentioned this pull request Feb 16, 2018
@nicoddemus nicoddemus merged commit b1abe5d into pytest-dev:features Feb 16, 2018
@nicoddemus
Copy link
Member

Thanks @uSpike!

@blueyed
Copy link
Contributor

blueyed commented Feb 19, 2018

I still think that it is confusing to have --ignore and --deselect now, and that support for foo.py::X should get added to --ignore instead.

--deselect requires the filename as a prefix already, and therefore is only a specialized version of --ignore, isn't it?

@twmr
Copy link
Contributor

twmr commented Feb 20, 2018

I guess that it makes sense to mentioned in the docs what the differences between --ignore and --deselect are

@uSpike uSpike deleted the deselect_cli branch February 20, 2018 16:23
@uSpike
Copy link
Member Author

uSpike commented Feb 20, 2018

--ignore is not exactly the same as --deselect. Paths that are excluded by --ignore are not reported anywhere via pytest_deselected or on the CLI. Items that are deselected by --deselect are passed to pytest_deselected and reported as so on the CLI.

I'm not sure if it's more or less confusing with --ignore if things that look like paths are not considered for collection at all, and things that look like node ids are deselected. It's a simple implementation and I'd be happy to submit another PR if there's interest.

@uSpike
Copy link
Member Author

uSpike commented Feb 20, 2018

I have a working implementation here: uSpike@8b98792

@blueyed
Copy link
Contributor

blueyed commented Feb 19, 2020

@uSpike
Thanks for following up with merging this with --ignore, and sorry for having missed it previously.
Would you like to follow up on that? (given that there just was confusion with --deselect not handling paths)

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.

6 participants