john-bodley commented on code in PR #24488:
URL: https://github.com/apache/superset/pull/24488#discussion_r1239092173


##########
tests/integration_tests/sqla_models_tests.py:
##########
@@ -189,11 +189,6 @@ def test_extra_cache_keys(self, flask_g):
         self.assertTrue(table3.has_extra_cache_key_calls(query_obj))
         assert extra_cache_keys == ["abc"]
 
-        # Cleanup

Review Comment:
   CI was failing for PostgreSQL with SQLAlchemy error which would indicate 
that we're trying to delete objects which previously weren't committed to the 
database. Given how these tables are constructed, 
   
   ```python
   table3 = SqlaTable(
       table_name="test_has_no_extra_cache_keys_table",
       sql=query,
       database=get_example_database(),
   )
   ```
   
   and there's no clear add/merge call it seems that SQLAlchemy's viewpoint is 
right, i.e., there's no cleanup required. If I'm right then this is a win for 
SIP-92. I suspect throughout the code and tests we're doing numerous database 
operations inefficiently with superfluous commits, not rolling back test state, 
etc. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to