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

Update tests to actually treat 'flask' as optional #201

Merged
merged 2 commits into from
Dec 6, 2024

Conversation

sirosen
Copy link
Member

@sirosen sirosen commented Dec 5, 2024

  1. Add a tox factor, flask, for installing flask via the flask extra
  2. Update the tox matrix to run with and without -flask in addition to -minimum_flask
  3. Make test collection ignore the flask/ test dir if flask can't be imported (also, rename the matching tests to tests/flask/)
  4. Minor changes in those tests so that the conftest.py won't fail during test collection

There's a long-standing inconsistency in that the tests have always required flask even though the library declares it to be an optional dependency.
We may soon have an actual usage of this library (an AP CLI client) which doesn't need flask, so resolving this one way or the other will help ensure that installs don't have broken imports if flask is missing.

1. Add a tox factor, `flask`, for installing flask via the `flask`
   extra
2. Update the tox matrix to run with and without `-flask` in addition
   to `-minimum_flask`
3. Make test collection ignore the `flask/` test dir if `flask` can't
   be imported (also, rename the matching tests to `tests/flask/`)
4. Minor changes in those tests so that the `conftest.py` won't fail
   during test collection
Copy link
Member

@kurtmckee kurtmckee left a comment

Choose a reason for hiding this comment

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

I think I was expecting some tests to be marked with a skipif or skipunlessimport (or whatever it's called), or to have a custom pytest marker associated with the Flask tests.

Is that what the collect_ignore in conftest.py is doing? If so, then I've learned a very neat new thing!

try:
import flask

assert flask
Copy link
Member

Choose a reason for hiding this comment

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

What does assert flask do?

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, I did that on the fly for flake8's F401. Let me replace with a noqa comment.

@sirosen
Copy link
Member Author

sirosen commented Dec 5, 2024

Yeah, I tried out the collect_ignore instead of a skip marker or similar because I wanted to avoid even trying to load those modules (and dealing with their imports). It's new to me, and we can obviously always switch to a skip marker if we find it doesn't work well.

One interesting thing is that even with that collect option set, the conftest.py in the relevant subdirectory gets loaded. To me, that was a big surprise! So I wouldn't call this a "new and recommended pattern" yet -- just a different way of doing things which seemed to fit this use case nicely.

@sirosen sirosen merged commit 702db8e into globus:main Dec 6, 2024
6 checks passed
@sirosen sirosen deleted the treat-flask-as-optional branch December 6, 2024 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants