-
-
Notifications
You must be signed in to change notification settings - Fork 901
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
add docs about testing #1171
Comments
I don't think this code snippet is sufficient restore the functionality most people used from sqlalchemy 1. If any of your code or tests rely on rollback inside the test, then this will just rollback you entire session. Basically any call to It's also painful because you can't override the behaviour via events either. For example trying to use retval=True and stop a commit or modify the behaviour doesn't work. |
It sounds like you're talking about a difference in behavior between SQLAlchemy 1.4 and 2.0. That's not what this is addressing, this is addressing how to isolate each test so that it doesn't modify the database (it's also not trying to be comprehensive to every potential way SQLAlchemy can be used). If there's something that doesn't work in SQLAlchemy 2 that you think should still work, that sounds like something you should report to SQLAlchemy. |
This is a fair response. And nor should flask_sqlalchemy/you have to handle changes in behaviour in sqlalchemy2. I should also be including But I still don't believe your snippet is sufficient for the majority of people trying to test their application. For example if you include |
But yes I am speaking about sqlalchemy 2. Sorry that was probably not clear. Your documentation is sufficient for sqlalchemy 1. |
This snippet works as written for SQLAlchemy 1.4 and 2.0. Commits in views are persisted during each test, but are not present in subsequent tests. |
You are correct. I wrote up the above with simple test cases. I overlook/expected By calling In any case, I retract my comments. It works, is simple, and is possible to put in a test fixture. Which the solution I have been working with is not so elegant. Cheers! |
I used the suggested code in a fixture, but after 20 tests, an error is thrown:
It look like the connections are not closed properly. Edit: David edited my code snipet, so i don't dare to rewrite it, but the error was on my side: i made the mistake of creating the app in the same fixture, so for each test, a new app was instancied, moving the app creation in a session scoped fixture fixed the problem. Thank you |
If anyone else was like me and had issues migrating their test suite to Flask-SQLAlchemy 3.1 / SQLAlchemy 2.0, I created a minimum working example which I used to figure out why my errors were happening. My main issue was ensuring tests could run in rollback-able transactions. https://github.com/gmassman/fsa-rollback-per-test-example To be clear, Flask-SQLAlchemy works perfectly well with pytest. However, it doesn't work well with some of the older patterns that might have been promoted in the past (e.g. creating a |
In order to get this working in code that test views with But doing so results in some warnings:
@pytest.fixture(autouse=True, scope="function")
def database(app):
# https://github.com/pallets-eco/flask-sqlalchemy/issues/1171
with app.app_context():
engines = db.engines
engine_cleanup = []
for key, engine in engines.items():
connection = engine.connect()
transaction = connection.begin_nested()
engines[key] = connection
engine_cleanup.append((key, engine, connection, transaction))
yield db
for key, engine, connection, transaction in engine_cleanup:
with warnings.catch_warnings():
warnings.simplefilter("ignore")
transaction.rollback()
connection.close()
engines[key] = engine (dumb example) but it's pretty common you might have some code that does something like: @index.put("/user")
def update_email():
user = db.session.get(User, request.json["id"])
user.email = request.json["email"]
if "@" not in user.email:
db.session.rollback()
return "error", 400
db.session.commit()
return "ok" def test_change_email(client):
user = User(email="[email protected]")
db.session.add(user)
db.session.commit()
# causes a warning
resp = client.put("/user", json={"id": user.id, "email": "haha"})
assert resp.status_code == 400
assert resp.text == "error"
assert user.email == "[email protected]"
resp = client.put("/user", json={"id": user.id, "email": "[email protected]"})
assert resp.status_code == 200
assert resp.text == "ok"
assert user.email == "[email protected]" |
This comment was marked as outdated.
This comment was marked as outdated.
@gmassman I'm hiding your reply because it has an even worse bug that's immediately apparent: you're pushing an app context then yielding, so the context is active during the entire test session. This is explicitly warned against in multiple places, you must not keep an active context around an individual test or the session. Only push a context for exactly as long as needed to do a specific piece of work. Perhaps your code does fix some shortcoming, but it's otherwise incorrect. You might be better off explaining what specifically you needed to add to the previous suggested code and why. |
Got it, thanks for the heads up! I wasn't aware this was a discouraged pattern. It's been part of our conftest.py for many years at this point, so I'm happy to remove it and handle app_context the right way. I'm sure with this removal I can get the previous code snippets to work. |
The fixtures that push transactions use a lot of objects that are either contextmanagers themselves or fit closing I wonder if using a exitstack wouldn't be of help |
I'm seeing this in so many examples/tutorials on how setup tests for flask+sqlachademy. I am however struggling to get this pattern to work. I'm using Flask 3.1, SQLAlchemy 2.0. I'm trying to use flask-sqlalchemy-lite (0.1.0, which pulls in flask-sqlalchemy 3.1) but from what I gather, this pattern described here should work for both flask-sqlalchemy and flask-sqlalchemy-lite? In my app I'm not seeing the database changes done in the test actually roll back. I'm guessing that I'm missing something. I'm brand new to python flask and sqlalchemy. Previously I've used PHP and Laravel for web projects, pretty opinionated framework, but it has the benefit of scaffolding complete app example with tests that rolls back out of the box. Since so many articles/tutorials is yielding within an app context, exactly what the flask-sqlalchemy docs days not to do when testing routes I tried setting up a minimal flask application to work my way through to what is the correct solution: But I'm not getting the rollbacks to work. My test fixture is: In this particular example I'm not even testing routes yet, just testing the models and trying to get them to rollback between runs: Is anyone able to spot why my setup isn't working? I'm willing to "pay back" by opening a PR afterword with adjustments to the docs to further aid newcomers like me. |
I'm not sure what the removed comment referred to, but I was looking at the link you posted before it. That is doing what isn't recommended, right? This is making an app context open for the whole duration of the test-run? |
@thomthom I pushed up some changes to my example app that you may find helpful. I modified the code you linked so app_context is pushed per test instead of per session which is what @davidism said was an anti-pattern. Here's a link to the latest file. Also zzzeek gave me some help to get transactional database fixtures working. See that link for more information. |
@gmassman I'm sorry, I know this is confusing, but you're still using the context incorrectly: https://github.com/gmassman/fsa-rollback-per-test-example/blob/7dd6f11236559391a72f6d7d0734f679092d5042/test/conftest.py#L27-L31 You must only push the app context for exactly as long as you need it. You're pushing it for an entire test, which means it will cause issues with any requests made inside that test. You must only push it around the database stuff you do, nothing more. Similarly, there is no reason to push the client unless you actually need to preserve the contexts after requests, which you almost never need to do. |
You all might be interested in Flask-SQLAlchemy-Lite as well. It's written by me and maintained in Pallets-Eco just like Flask-SQLAlchemy. It's intended to be much more lightweight than this extension, only adding some configuration and session management, no custom base model or session binding. You use normal SQLAlchemy models, normal SQLAlchemy session binding, etc. I also wrote extensive documentation there on testing patterns: https://flask-sqlalchemy-lite.readthedocs.io/en/latest/testing/ |
Gotcha, thanks for the help. I wasn't aware the test_client push wasn't necessary. Yes, testing with Flask-SQLAlchemy is quite confusing, so I appreciate your guidance and documentation links. Flask-SQLAlchemy-Lite looks like a good option for my needs, so I'll likely transition to it eventually. Appreciate your time maintaining the projects and supporting the community! |
I've been trying Anyone able to see what I'm doing wrong? |
Mainly, show how to isolated the database per test, so that changes are rolled back without having to re-create the database each time. The pytest-flask-sqlalchemy package provided this, but stopped working when Flask-SQAlchemy 3 changed how engines and the session were handled. After investigation, it seems like there's a much simpler way to do this.
I'll at least document the following code, but it's probably better to include it as a patch function in a
flask_sqlalchemy.test
module. Could possibly add pytest fixtures as well that used it.Unlike the linked package, this is all that's needed and supports the multiple binds feature.
The text was updated successfully, but these errors were encountered: