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

Incorrect usage of Flask's app context during testing #90

Closed
rsyring opened this issue Oct 22, 2017 · 6 comments · Fixed by #93
Closed

Incorrect usage of Flask's app context during testing #90

rsyring opened this issue Oct 22, 2017 · 6 comments · Fixed by #93

Comments

@rsyring
Copy link
Member

rsyring commented Oct 22, 2017

Our keg.testing:ContextManager is designed to cache the application instance as well as the app context. However, after reading the Flask docs on app context, it has become obvious that I'm using the app context wrong. Despite it's name, it's not intended to have the same life cycle as the app instance. It's intended to have a lifetime almost the same as the request context.

This is actually the underlying reason behind the bug I thought Flask-WTF had: wtforms/flask-wtf#301

It looks like some kind of pytest fixture support is what we really need in order to make the app instance available to our tests instead of relying on flask.current_app.

Some good ideas here:

https://github.com/pytest-dev/pytest-flask

rsyring added a commit that referenced this issue Oct 23, 2017
Don't create an application context during testing.  See #90 for
more info on the reason why this was problematic.  Adjust test
fixtures to compensate.

BREAKING CHANGE
---------------

No longer initializing an app context means that flask.current_app
as well as keg.db.db operations may fail where they did not
previously.  You will need to adjust your test fixtures to either
get the app instance directly or create the app context as needed.

The changes to our tests in this commit will provide good examples
of different kinds of fixture usage that can be used to work around
this change.
rsyring added a commit that referenced this issue Oct 23, 2017
Don't create an application context during testing.  See #90 for
more info on the reason why this was problematic.  Adjust test
fixtures to compensate.

BREAKING CHANGE
---------------

No longer initializing an app context means that flask.current_app
as well as keg.db.db operations may fail where they did not
previously.  You will need to adjust your test fixtures to either
get the app instance directly or create the app context as needed.

The changes to our tests in this commit will provide good examples
of different kinds of fixture usage that can be used to work around
this change.
@rsyring
Copy link
Member Author

rsyring commented Aug 28, 2018

A lot of our apps assume an app context is going to be available in setup_class() and setup fixtures for things like entity usage. However, my attempt to fix Keg for this issue breaks those assumptions and would require a lot of work from the app developers to fix their tests. I've been hoping pytest would address this, but the issue hasn't received much attention. There is, however, a plugin now available:

pytest-dev/pytest#517 (comment)

@rsyring
Copy link
Member Author

rsyring commented Jan 23, 2019

pytest 4.2 will have a fix for us:

pytest-dev/pytest#517 (comment)

This was referenced Oct 31, 2019
nZac pushed a commit that referenced this issue Nov 21, 2019
Don't create an application context during testing.  See #90 for
more info on the reason why this was problematic.  Adjust test
fixtures to compensate.

BREAKING CHANGE
---------------

No longer initializing an app context means that flask.current_app
as well as keg.db.db operations may fail where they did not
previously.  You will need to adjust your test fixtures to either
get the app instance directly or create the app context as needed.

The changes to our tests in this commit will provide good examples
of different kinds of fixture usage that can be used to work around
this change.
guruofgentoo added a commit to level12/keg-auth that referenced this issue Dec 7, 2022
- flask-login now uses flask.g to cache the logged-in user
- level12/keg#90 needs to be resolved before we can remove the workaround
@guruofgentoo
Copy link
Member

This is now affecting both CSRF and how flask-login caches the logged-in user on flask.g. I took a look at this during this maintenance cycle to see if I could wrap this up, and there's a few things at play here that make it non-trivial to finish up. I've punted for this cycle of dependency updates.

Taking keg-auth's test suite as a sample, since it doesn't have the same type of custom app setup stuff going on during tests:

  • it is possible to put in a session-scope autouse fixture in conftest.py, that pushes an app context to satisfy all of our setup_class usage
  • a number of test decorators, like mock or skip marks, can be broken if there's a reference to something needing the app context
    • I believe all of those usages can get moved to inside the test method
  • the main reason I punted was that there's still some weirdness around how the CSRF session token is applied for the webtest client. It will seem like tests are functioning normally, then if the session is cleared (like on logout), the token is lost, and there doesn't seem to be a way to get it back in there.
    • running more than one webtest client at a time seems to run into problems with this as well
  • flask-webtest does not push an app context before running requests. If that's how it works in general (that we get a fresh app context and request context for each request), though, IMO it really should be doing that
    • we could make that update, as maintainers of flask-webtest now
    • testing with that app context push in place did resolve a number of issues around how these requests were working
    • the side effect is that ORM objects in views are no longer on the same session as the tests themselves. Which is not a bad thing, but breaks the assumptions in some of our tests.

guruofgentoo added a commit that referenced this issue Jan 4, 2023
Don't create an application context during testing.  See #90 for
more info on the reason why this was problematic.  Adjust test
fixtures to compensate.

BREAKING CHANGE
---------------

No longer initializing an app context means that flask.current_app
as well as keg.db.db operations may fail where they did not
previously.  You will need to adjust your test fixtures to either
get the app instance directly or create the app context as needed.

The changes to our tests in this commit will provide good examples
of different kinds of fixture usage that can be used to work around
this change.
@rsyring
Copy link
Member Author

rsyring commented Jan 4, 2023

I haven't read all of your comment above yet but I wanted to point out that I was planning on solving some of the problems with this by using app/db fixtures in the setup_*() methods. Felt like a reasonably quick find/replace operation to update all our usages. But, fixtures aren't supported in those methods currently and I got no where trying to get them to address it. It is addressable, just needs some time: pytest-dev/pytest#5289

Also, maybe that's not the best solution.

@guruofgentoo
Copy link
Member

guruofgentoo commented Jan 4, 2023

I'm ending up doing two things:

  • Most of the setup weirdness can be handled by having an app context pushed for loading tests. I.e. a fixture is run at module setup time. Note this is after test gathering happens, so flask.current_app isn't available for mocking decorators, but at least the db session is available.
  • Second, and the larger drawback here that makes the test suite different from running the app in production: nothing is pushing an app context on each request.
    • flask-webtest is the lib that does all the flask stack setup for webtest, so it seems like that's where it should happen
    • My intent is to wrap https://github.com/level12/flask-webtest/blob/master/flask_webtest.py#L220 with an app context push, from the app used to create the client
    • In my testing with keg-auth (linked above, which is pretty similar to how we test apps), that solved all of the flask.g-related issues

FWIW, I have the keg-auth tests working locally with these updates and the branch linked above. I think that's at least the first stage in moving this direction, but if I'm missing something, I'm all ears.

@guruofgentoo
Copy link
Member

Note, level12/flask-webtest#18 is required for the current solution.

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 a pull request may close this issue.

2 participants