pallets-eco / flask-sqlalchemy

Adds SQLAlchemy support to Flask
https://flask-sqlalchemy.readthedocs.io
BSD 3-Clause "New" or "Revised" License
4.22k stars 900 forks source link

add docs about testing #1171

Open davidism opened 1 year ago

davidism commented 1 year ago

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.

with a.app_context():
    engines = db.engines

engine_cleanup = []

for key, engine in engines.items():
    connection = engine.connect()
    transaction = connection.begin()
    engines[key] = connection
    engine_cleanup.append((key, engine, connection, transaction))

yield a

for key, engine, connection, transaction in engine_cleanup:
    transaction.rollback()
    connection.close()
    engines[key] = engine

Unlike the linked package, this is all that's needed and supports the multiple binds feature.

ElementalWarrior commented 1 year ago

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 session.rollback() or session.get_transaction() will act on the topmost transaction in sqlalchemy 2.

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.

davidism commented 1 year ago

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.

ElementalWarrior commented 1 year ago

it's also not trying to be comprehensive to every potential way SQLAlchemy can be used

This is a fair response. And nor should flask_sqlalchemy/you have to handle changes in behaviour in sqlalchemy2.

I should also be including session.commit() in my comment above.

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 session.commit() in your application, then call that in a test, when you hit that yield from your original post, it will not have anything to rollback, because the root transaction would have been committed.

ElementalWarrior commented 1 year ago

But yes I am speaking about sqlalchemy 2. Sorry that was probably not clear. Your documentation is sufficient for sqlalchemy 1.

davidism commented 1 year ago

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.

ElementalWarrior commented 1 year ago

You are correct. I wrote up the above with simple test cases.

I overlook/expected connection in your setup to behave similar to how sqlalchemy session does if you don't wrap the logic in connection.begin().

By calling connection.begin() before doing any db.session... logic it seems to mostly work. I tried it on a larger codebase and it's unhappy about an instance being detached from a session, but that's a separate topic.

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!

yobuntu commented 1 year ago

I used the suggested code in a fixture, but after 20 tests, an error is thrown:

sqlalchemy.exc.OperationalError: (psycopg2.OperationalError) connection to server at "localhost" (127.0.0.1), port 5432 failed: FATAL:  les emplacements de connexions restants sont réservés pour les connexions
superutilisateur non relatif à la réplication

It look like the connections are not closed properly.

Edit: Sorry every one,

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

gmassman commented 9 months ago

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 db fixture a la pytest-flask-sqlalchemy).

Jackevansevo commented 3 weeks ago

In order to get this working in code that test views with db.session.rollback() logic I had to use a begin_nested

But doing so results in some warnings:

test_app.py::test_thing
  /Users/jack/code/example/test_app.py:49: SAWarning: nested transaction already deassociated from connection
    transaction.rollback()
@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="test@google.com")
    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 == "test@google.com"

    resp = client.put("/user", json={"id": user.id, "email": "person@google.com"})
    assert resp.status_code == 200
    assert resp.text == "ok"
    assert user.email == "person@google.com"