apache / superset

Apache Superset is a Data Visualization and Data Exploration Platform
https://superset.apache.org/
Apache License 2.0
62.8k stars 13.87k forks source link

feat: turn off autoflush on integration tests #30394

Open betodealmeida opened 1 month ago

betodealmeida commented 1 month ago

SUMMARY

https://github.com/apache/superset/pull/24969 introduced the @transaction decorator, to be used in commands to ensure that commits are explicit and happen only once in the lifetime of a request, preventing eagers commits. The PR failed to catch a few cases where legacy API endpoints were not using commands, resulting in successful requests that wouldn't be persisted to the database. These were fixed in https://github.com/apache/superset/pull/30215.

Why did our integration tests fail to catch these endpoints that were not committing? Tests failed because by default SQLAlchemy will autoflush before running a SELECT. In real world, favoriting a chart and verifying that it was favorited are two separate requests. If the first one fails to commit, the second one won't see a change. But our integration tests share the same global session (db.session), so that an insert followed by a select will result in flush between them, and the non-committed data will be seen even though we use READ COMMITTED as the isolation level.

To prevent this from happening again, this PR turns off the autoflush in the integration tests.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

michael-s-molina commented 1 month ago

@betodealmeida Another idea would be to keep autoflush in our tests and check the session for pending commits/rollbacks after our tests run. This can be done globally and would work for current and new code.