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

Make zope a semi-optional test dependency #685

Merged
merged 4 commits into from
Sep 3, 2020

Conversation

pganssle
Copy link
Member

@pganssle pganssle commented Sep 2, 2020

For the admittedly small number of people who don't care about support for zope.interface, it is unnecessary extra work to support even a test-only dependency on zope.

The zope-specific tests are relatively contained, however, and with judicious use of pytest fixtures, we can make it so that zope is not a required dependency when skipping tests marked with pytest.mark.zope.

This allows users to test everything but the zope-specific tests with no zope dependency with:

$ TOX_AP_TEST_EXTRAS="tests_no_zope" tox -e <env> -- -m 'not zope'

I realize that this is unusual and maybe not a maintenance burden you want to carry upstream, but it would smooth the process of importing attrs upstream into a certain large company's monorepo... I'm fine with a response that's basically "Enough with your weird nonsense, Paul".

I didn't bother with documentation or a news fragment yet because I figured there's a high likelihood that this wouldn't be accepted.

Pull Request Check List

  • Changes (and possible deprecations) have news fragments in changelog.d.

@hynek
Copy link
Member

hynek commented Sep 2, 2020

Would you mind inverting the logic and just make the Zope tests conditional on the presence of Zope.interface? That seems to be the more common approach nowadays anyways.

@pganssle
Copy link
Member Author

pganssle commented Sep 2, 2020

Would you mind inverting the logic and just make the Zope tests conditional on the presence of Zope.interface? That seems to be the more common approach nowadays anyways.

Yeah, that's easy enough — the main difference between that approach and this one is that with this approach, it would be very hard to accidentally disable the zope-related tests, whereas if you always disable the zope-related testing whenever zope.interface isn't present, anyone who fails to declare a test dependency on zope.interface will silently start skipping tests.

I figured this is a niche enough thing to want that it's better to require enthusiastic opt-in, but maybe it's not that big a deal if these tests sometimes get accidentally disabled in downstream testers?

@hynek
Copy link
Member

hynek commented Sep 2, 2020

I don’t think anybody cares at all TBH. We’re just dragging it along for backward-compat.

For the admittedly small number of people who don't care about support
for `zope.interface`, it is unnecessary extra work to support even a
test-only dependency on `zope`.

The `zope`-specific tests are relatively contained, however, and with
judicious use of pytest fixtures, we can make it so that `zope` is not a
required dependency when skipping tests marked with `pytest.mark.zope`.

This allows users to test everything but the `zope`-specific tests with
no `zope` dependency with:

$ TOX_AP_TEST_EXTRAS="tests_no_zope" tox -e <env> -- -m 'not zope'
This is a little more error-prone, but it's much more convenient to
invoke, since it doesn't involve skipping the "zope" marker.
@pganssle
Copy link
Member Author

pganssle commented Sep 2, 2020

OK, I've changed it to an auto-skipping fixture and added some docstrings and a changelog. If you think there's somewhere else that this should be explicitly documented, let me know. It didn't seem like it would fit much into CONTRIBUTING.rst, since this is more aimed at downstream redistributors than at contributors.

@hynek
Copy link
Member

hynek commented Sep 3, 2020

Thanks!

@hynek hynek merged commit dfb2ee2 into python-attrs:master Sep 3, 2020
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.

2 participants