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.

Sign up for GitHub

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

Jump to bottom

allow overriding pytest functional arguments with posargs #1732

Closed
wants to merge 1 commit into from
Closed

allow overriding pytest functional arguments with posargs #1732

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ extras =
vagrant
commands =
unit: pytest test/unit/ --cov={toxinidir}/molecule/ --no-cov-on-fail {posargs}
functional: pytest test/functional/ {posargs}
functional: pytest {posargs:test/functional/}
Copy link
Member

Choose a reason for hiding this comment

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

There's no point in doing this since currently -- -k ... syntax works. With this, it'll match tests from unit test suite as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason why pytest has both -k and positional arguments is the same reason why this patch is proposed. For instance, it allows for tox -- test/test.py to run all test from a single test file. Achieving the same result with the -k option would be impractical.

Copy link
Member

Choose a reason for hiding this comment

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

I see why you'd want that and it's a natural wish.

Current tox.ini combines too much test modes in one section. I think it's an antipattern and should be improved.

Now, unit and functional factors correspond to running unit tests and functional tests respectively. But the proposed change advertises abusing that semantics which is far from being intuitive from the UX point of view.

I propose the following (thoughts, which I was having in mind for a while now):

  1. separate these sections to make a clear distinction between their purposes
  2. have a dedicated section for running whatever you want with pytest
  3. [maybe] remove that tags plugin, I haven't heard about anyone using it, it complicates things a lot and it's probably possible to achieve the same result in other ways...

P.S. If you want, I can do (1) and (2) since I've got some experience with tox internals.

Copy link
Member

Choose a reason for hiding this comment

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

P.P.S. @ssbarnea has submitted an unfinished attempt to improve other aspects of tox config @
cb4ad30 via #1547 in past. This is also worth exploring...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the proposed change advertises abusing that semantics which is far from being intuitive from the UX point of view.

I respectfully disagree. This is a very common and convenient pattern.

Copy link
Contributor

Choose a reason for hiding this comment

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

You've raised important points @webknjaz. If I read this correctly, your comment in #1732 (comment) shows us a more sustainable plan for our Tox usage. That's great and I would like to see that in a separate issue to share knowledge of what needs to be done and who is doing it.

For the purposes of this PR: this change might not be taking us in the direction of what you've desribed but it is making it more convenient to run specific tests for contributors in the right here and now. That's important and for that reason, I'd like to see this change merged as a 'low road' to making Tox easier to work with. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

@lwm

Feel free to overrule my judgment, for now. We can fix it later, after all.

OTOH after this change, it will be possible running unit tests from within functional factor which makes the existence of unit factor questionable.

P.S. on

is making it more convenient to run specific tests

it is already possible:

tox -e unit -- -k test_unit_smth
tox -e functional -- -k test_functional_smth

and after this change it will be

tox -e functional -- -k test_unit_smth
tox -e functional -- -k test_functional_smth
tox -e functional -- test/unit/test_unit_thing.py::test_unit_smth

and if you apply the suggestion below (https://github.com/ansible/molecule/pull/1732/files#r257429626) if will look like

tox -e pytest -- -k test_unit_smth
tox -e pytest -- -k test_functional_smth
tox -e pytest -- test/unit/test_unit_thing.py::test_unit_smth
tox -e pytest -- test/functional/test_func_thing.py::test_functional_smth

Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to overrule

Overruling is not productive behaviour.

Copy link
Member

Choose a reason for hiding this comment

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

Not if I explicitly allowed you to :)

In this case, it may be more productive as you need to keep going with improvements and I don't have any time involvement guarantees. I just wanted to share/document my vision but not block anyone.

lint: flake8
lint: yamllint -s test/ molecule/

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[testenv:pytest]
commands =
pytest {posargs}

Expand Down