feat: import dsl add integration test#34498
Conversation
Pyrefly DiffNo changes detected. |
1 similar comment
Pyrefly DiffNo changes detected. |
Pyrefly DiffNo changes detected. |
1 similar comment
Pyrefly DiffNo changes detected. |
Pyrefly DiffNo changes detected. |
Pyrefly DiffNo changes detected. |
There was a problem hiding this comment.
Pull request overview
Adds/updates integration coverage around App DSL import in the testcontainers suite, aiming to prevent regressions related to the session type used during imports.
Changes:
- Use
CURRENT_DSL_VERSIONin test YAML generation to avoid hardcoding the DSL version. - Add a new integration test intended to validate chat-app import behavior when using an engine-bound SQLAlchemy
Session.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| with ``Session(db.engine)`` in AppImportApi.post(). The previous | ||
| approach returned a ``SessionTransaction`` instead of a ``Session``, | ||
| which broke AppDslService operations such as ``session.scalar()`` and | ||
| ``session.add()``. |
There was a problem hiding this comment.
The docstring claims sessionmaker(db.engine).begin() returns a SessionTransaction (and not a Session). That’s only true when calling .begin() directly; when used as with sessionmaker(...).begin() as session, the as binding is a Session. Please reword this explanation to accurately describe the failure mode being guarded against (e.g., passing a SessionTransaction into AppDslService).
| with ``Session(db.engine)`` in AppImportApi.post(). The previous | |
| approach returned a ``SessionTransaction`` instead of a ``Session``, | |
| which broke AppDslService operations such as ``session.scalar()`` and | |
| ``session.add()``. | |
| with ``Session(db.engine)`` in AppImportApi.post(). The important | |
| behavior is that AppDslService must receive a real ``Session``. | |
| The previous pattern was problematic because it could result in | |
| passing a transaction-oriented object into AppDslService instead of | |
| a ``Session``, which broke operations such as ``session.scalar()`` | |
| and ``session.add()``. |
| yaml_content=yaml_content, | ||
| name="Engine-session import", | ||
| ) | ||
| session.commit() |
There was a problem hiding this comment.
This test manually calls session.commit() after import_app(), but AppImportApi.post() (which this test references) does not commit the Session(db.engine) before closing it. As written, the test can pass even if the real endpoint still drops uncommitted changes (e.g., the newly-added AppModelConfig). Consider either (1) exercising the actual import API route via the test client, or (2) removing the explicit commit here so the test matches production behavior and forces the service/controller to commit as needed.
| session.commit() |
Important
Fixes #<issue number>.Summary
fix #34497
Screenshots
Checklist
make lintandmake type-check(backend) andcd web && npx lint-staged(frontend) to appease the lint gods