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

Reorganize test requirements #957

Closed
wants to merge 1 commit into from
Closed

Reorganize test requirements #957

wants to merge 1 commit into from

Conversation

ssbarnea
Copy link
Member

@ssbarnea ssbarnea commented Aug 14, 2020

This enables running of tests w/o tox, with just pip install -e .[test] or similar, something that was not possible before.

Update: Please check https://discuss.python.org/t/is-it-preferable-to-add-test-dependensies-as-an-extra-require-instead-of-tox-ini/4984 thread if you have doubts if this approach may not be good.

@ssbarnea ssbarnea added skip-changelog Can be missed from the changelog. merge-it labels Aug 14, 2020
@ssbarnea ssbarnea marked this pull request as ready for review August 14, 2020 15:48
@ssbarnea ssbarnea changed the title Avoid incompatible pytest-xdist Reorganize test requirements Aug 14, 2020
@ssbarnea ssbarnea requested a review from awcrosby August 14, 2020 16:49
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

This env pins shouldn't be in extras. They should be in pip-compile lockfiles instead.

setup.cfg Outdated Show resolved Hide resolved
@ssbarnea
Copy link
Member Author

recheck

Copy link

@odyssey4me odyssey4me left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@greg-hellings greg-hellings left a comment

Choose a reason for hiding this comment

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

I'm in favor of not being permanently lashed to one particular tool

- Move test requirements from tox.ini to a test extra
- Removes unused test requirements
- Bumps test requirements to decrease change of running outdated
  version while developing, including avoidance of
  pytest-dev/pytest-cov#422
@webknjaz
Copy link
Member

@greg-hellings we are not talking about any tools here. @ssbarnea is trying to shove tests execution virtualenv deps into the metadata of a pip-installable distribution that is completely unrelated to setting up the test environment.

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Test deps should be (1) installable w/o the dist itself. They should be pinned (2). And those pins should be auto-updatable by tools like dependabot and similar. I don't see how adding them to dist metadata would help here. I'd say that it's harmful to add them to runtime deps. Besides, influencing setuptools in the virtualenv is not a good idea either.

@ssbarnea
Copy link
Member Author

Test deps should be (1) installable w/o the dist itself. They should be pinned (2). And those pins should be auto-updatable by tools like dependabot and similar. I don't see how adding them to dist metadata would help here. I'd say that it's harmful to add them to runtime deps. Besides, influencing setuptools in the virtualenv is not a good idea either.

I already considered that after you introduced them to docs. Still I have some concerns that too tight control over them would translate into few additional issues:

  • conflicts where a pinned set of reqs fails on specific platform (one not covered by CI)
  • extra noise from dependabot updates -- i really do not want to see dependabot updating setuptools every other day. Now I want to use a pinned version of setuptools, as it would prevent us from discovering bugs with newer ones while testing.
  • two extra files in repository root, a place already crowded

Note that I am not strongly against dependabot approach. I only stated my concerns and why I prefer using extras with range instead of pinning. Still, I am willing to accommodate other preferences. Lets see what others think, adding feedback-needed label.

@ssbarnea ssbarnea added the feedback-needed Divergent oppinions, additional feedback is desired in order to unblock it. label Aug 27, 2020
@ansible ansible deleted a comment from pabelanger Aug 27, 2020
@ssbarnea
Copy link
Member Author

Dropped in favour of #1011 alternative

@ssbarnea ssbarnea closed this Aug 27, 2020
@ssbarnea ssbarnea deleted the fix/test branch December 15, 2020 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback-needed Divergent oppinions, additional feedback is desired in order to unblock it. skip-changelog Can be missed from the changelog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants