tests: Fix testcase for flask-login 0.6.2

[AbdealiLoKo]

In flask-login 0.6.2 they have started using g to save the user. Ref: https://github.com/maxcountryman/flask-login/commit/359fb004f153635952a7d32ded1bbd6d74ec2561#

This is causing an issue with our tests because earlier the user was stored in a request context. But g is stored within an app context.

We need to explicitly clear the g to get this to work correctly

[AbdealiLoKo]

@marksteward - Fixed the testcase so we can start getting PRs merged. Note - I am not 100% clear on the difference between appcontext and requestcontext StackOverflow seems to indicate that every "api call" will always have a new appcontext.

The reason flask-login moved to g seems to be due to this warning: DeprecationWarning: '_request_ctx_stack' is deprecated and will be removed in Flask 2.3. Use 'g' to store data, or 'request_ctx' to access the current context. So they have moved to the recommended approach flask suggests

[marksteward]

Ugh, I see, flask-login has moved from the request to the app context in https://github.com/maxcountryman/flask-login/pull/691. We shouldn't need to grovel inside its internals, if we need it to load the user that means a code change, not just the test.

[AbdealiLoKo]

I looked at it a bit more. I don't think there is any issue with the codebase - it seems to be working fine . It seems to be an issue with the testcase setup - where a requestcontext is being created too early ? When I move the context creation where required:

context = self.app.test_request_context()
context.push()
# <--- Aded this here and removed self.context

self.login(user)
self.client.get(url_for('.test_simple_flush'))

# ---> Aded this here and removed self.context
context.pop()
context = None

It is working fine. So, it seems that within a "context" (I am not clear if request ctx or app ctx) - there is caching. The anonymous user was getting cached in the first commit() And the way the testcase was written was invoking the caching before the login() function was being called.

So, overall - the issue is that we were creating the contexts too early. I have made a login() contextmanager to make it easier to read and use in the tests without causing such issues

[AbdealiLoKo]

Also, FYI - I tested this works in both flask-login 0.6.0 and 0.6.2

[marksteward]

I'm still not sure this is the right direction to go. A test should generally try to demonstrate typical library usage, and this:

        with self.app.test_request_context(), self.app.test_client() as client:
...
            yield client
...
        with self.login(user) as client:
            client.get(url_for('.test_simple_flush'))

is not something I've seen anywhere else. Why should login be a contextmanager? If the user shouldn't be cached between requests, that's something we should take up with flask-login. If we need multiple app contexts per test, that should be explicitly done, not hidden inside login.

[AbdealiLoKo]

Ok ... what would you recommend we do ? Should we mark the test as a "xfail" and raise a fticket upstream ?

[marksteward]

Either work out the intent of the code, or wait until I've got a free evening to look at it (perhaps tomorrow).

[AbdealiLoKo]

Honestly - I am not able to follow the structure of the current way test are written I normally use fixtures and pytest-flask so it does setups/cleanups for me

So I will wait until tomorrow.

I feel like this seems to be blocking a lot of other good PRs with valid improvements to sqlalchemy-continuum though It would be great if you could consider merging this and I can add a FIXME comment that this test needs to be improved for the pytest/flask ninjas who may want to contribute

[marksteward]

It's blocking one PR that you just made. Please have a little patience.

[marksteward]

So it looks like Flask 2.2.2 has deprecated use of both app and request contexts (see https://github.com/pallets/flask/commit/e0dad45 and https://github.com/pallets/flask/commit/b46bfcf), and flask-login's change is to comply with this.

The good news is that using login_user (as we should) fixes it.

[marksteward]

Fixed by 8552a07

[AbdealiLoKo]

Perfect 👍 awesome