Dispose SQLAlchemy's engine in storage_tests/rdb_tests/test_storage.py#6338
Conversation
|
@fusawa-yugo Could you review this PR? |
|
could you please use |
|
@sawa3030 Could you review this PR? |
sawa3030
left a comment
There was a problem hiding this comment.
Thank you for the PR. I’ve left a minor comment. PTAL
| @contextmanager | ||
| def create_test_storage( | ||
| url: str = "sqlite:///:memory:", | ||
| engine_kwargs: dict[str, Any] | None = None, | ||
| skip_compatibility_check: bool = False, | ||
| skip_table_creation: bool = False, | ||
| ) -> Generator[RDBStorage, None, None]: | ||
| storage = RDBStorage( | ||
| url, | ||
| engine_kwargs=engine_kwargs, | ||
| skip_compatibility_check=skip_compatibility_check, | ||
| skip_table_creation=skip_table_creation, | ||
| ) | ||
| yield storage | ||
| storage.engine.dispose() |
There was a problem hiding this comment.
This might be slightly outside the scope of this PR, but could you move the function create_test_storage to the top of the file, before test_init? Since it’s used in the very first test (test_init), I think it would make the file a bit easier to follow.
There was a problem hiding this comment.
Thank you for the comment.
Indeed, I had been thinking about this as well, but I avoided including it in this PR to keep the diff minimal.
However, I completely agree that this function should be moved to the top of the file, so I will apply this change as you suggested. I also think it is unlikely that a standalone PR whose sole purpose is to move this function will be created in the future.
af10f13 to
e8d831e
Compare
|
Thank you for the comment. I've update the code accordingly. PTAL |
sawa3030
left a comment
There was a problem hiding this comment.
Thank you for the update. LGTM!
|
LGTM. |
|
This pull request has not seen any recent activity. |
Motivation
This PR is the follow-up on:
Engine#6303,and runs in parallel with::
storage_tests/test_with_server.py#6330, andstorage_tests/test_cached_storage.py#6337.The original PR did not cover all test cases. This PR addresses the remaining ones in
storage_tests/rdb_tests/test_storage.py.Important
With these PRs and this PR, all the tests creating
RDBStorageintest_storageshas been addressed.The remaining tests creating
RDBStorageis onlystudy_tests/test_study_summary.py, which creates theRDBStorageinstance for only two times and considered to be less likely to encounter the SQLAlchemy's issue.Description of the changes
create_test_storagecontext manager so thatstorage.engine.dispose()is executed every time.create_test_storagefor all tests.Note
To ensure that
dispose()is always called even when an assert fails, it should be invoked inside__exit__rather than at the end of the test function.Important
Most of the diff comes from increased indentation after introducing the context manager.