Opened 4 years ago
Closed 4 years ago
#32445 closed Cleanup/optimization (fixed)
LiveServerThreadTest.test_closes_connections() doesn't pass with non-in-memory SQLite
Reported by: | Chris Jerdonek | Owned by: | Chris Jerdonek |
---|---|---|---|
Component: | Testing framework | Version: | 3.1 |
Severity: | Normal | Keywords: | SQLite, is_usable, LiveServerThread |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
I noticed that LiveServerThreadTest.test_closes_connections()
doesn't seem to pass when using non-in-memory SQLite. It's only configured to be skipped for in-memory SQLite. It doesn't pass because the test has the following line:
self.assertFalse(conn.is_usable())
But SQLite's is_usable()
has a hard-coded return value of True
:
https://github.com/django/django/blob/3fa1ed53be370f4b1a94d78b56ff30d23b131623/django/db/backends/sqlite3/base.py#L387-L388
I guess this means that Django's CI doesn't check non-in-memory SQLite. If it's true that Django doesn't check this case, I'm not sure if that means it doesn't need to be fixed.
Change History (10)
comment:1 by , 4 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Uncategorized → Cleanup/optimization |
follow-up: 3 comment:2 by , 4 years ago
Just to be totally clear, would @skipUnlessDBFeature('test_db_allows_multiple_connections')
mean that the test is or isn't skipped with non-in-memory SQLite?
comment:3 by , 4 years ago
Replying to Chris Jerdonek:
Just to be totally clear, would
@skipUnlessDBFeature('test_db_allows_multiple_connections')
mean that the test is or isn't skipped with non-in-memory SQLite?
It will always be skipped on SQLite, so also with non-in-memory database.
follow-up: 5 comment:4 by , 4 years ago
It will always be skipped on SQLite, so also with non-in-memory database.
Okay, that's what I thought. Why do you think the test should always be skipped on SQLite as opposed to making the test work also for non-in-memory SQLite? It seems like making it work would be the better outcome since it would cover more scenarios where the test makes sense.
comment:5 by , 4 years ago
Replying to Chris Jerdonek:
Okay, that's what I thought. Why do you think the test should always be skipped on SQLite as opposed to making the test work also for non-in-memory SQLite? It seems like making it work would be the better outcome since it would cover more scenarios where the test makes sense.
Agreed, making the test work would be better, but I'm sure how to do this. I would be happy to review a patch, if you have an idea.
comment:6 by , 4 years ago
Agreed, thank you. I have some ideas, but not 100% sure it will work out.
comment:7 by , 4 years ago
Okay, I came up with a solution for getting the test to work with non-in-memory SQLite. The main idea is to use TransactionTestCase
instead of TestCase
.
comment:8 by , 4 years ago
Has patch: | set |
---|
comment:9 by , 4 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Triage Stage: | Accepted → Ready for checkin |
That's True, but I think it's still worth fixing. There is an interesting discussion about this test in the original PR. I think we should remove
test_closes_connections()
fromdjango_test_skips()
and use@skipUnlessDBFeature('test_db_allows_multiple_connections')
instead.