#27079 closed Cleanup/optimization (fixed)
Refactor LiveServerPort tests to not make extra calls to setUpClass() and tearDownClass()
| Reported by: | Chris Jerdonek | Owned by: | Jacob Walls |
|---|---|---|---|
| Component: | Testing framework | Version: | 1.10 |
| Severity: | Normal | Keywords: | |
| 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
Currently, LiveServerPort.test_port_bind() calls LiveServerTestCase.setUpClass() and LiveServerTestCase.tearDownClass() twice (to create two live server threads -- see here for the code). On the tear-down side, this has the unwanted side effect of setting conn.allow_thread_sharing to False before all live server threads are done with their connections.
The test should really only be calling the full setUpClass() and tearDownClass() once (but still doing the partial setup and tear-down that it needs for the purposes of its test).
Change History (15)
comment:1 by , 8 years ago
| Component: | Uncategorized → Testing framework |
|---|---|
| Triage Stage: | Unreviewed → Accepted |
| Type: | Uncategorized → Bug |
comment:2 by , 8 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
follow-up: 5 comment:3 by , 8 years ago
| Summary: | LiveServerPort.test_port_bind() sets allow_thread_sharing to False before all server threads are done → Refactor LiveServerPort.test_port_bind() not to call setUpClass() twice |
|---|
comment:4 by , 4 years ago
| Owner: | removed |
|---|---|
| Status: | assigned → new |
comment:5 by , 3 years ago
Replying to Chris Jerdonek:
This merged pull request removed calling
tearDownClass()twice (because it blocked the resolution of that issue), so I'm retitling this ticket to reflect the updated situation.
Chris, is it the case that the changes you referred to were undone in 8c775391b78b2a4a2b57c5e89ed4888f36aada4b? Perhaps we could revert to the original issue title (and include the test that follows it)?
comment:6 by , 3 years ago
Chris, is it the case that the changes you referred to were undone in 8c775391b78b2a4a2b57c5e89ed4888f36aada4b? Perhaps we could revert to the original issue title (and include the test that follows it)?
Nice observation, Jacob. More specifically, it was done in these lines and below.
Do you want to go ahead with what you suggest? It could also help to comment on that PR so the connection between the tickets is more noticeable.
comment:7 by , 3 years ago
| Summary: | Refactor LiveServerPort.test_port_bind() not to call setUpClass() twice → LiveServerPort test methods set allow_thread_sharing to False before all server threads are done |
|---|
Back to the original state of play after changes in #28478. My understanding is that we should tailor the setup and cleanup in these two tests rather than make extra calls to the class methods.
comment:8 by , 3 years ago
| Owner: | set to |
|---|---|
| Status: | new → assigned |
| Summary: | LiveServerPort test methods set allow_thread_sharing to False before all server threads are done → Refactor LiveServerPort tests to not make extra calls to setUpClass() and tearDownClass() |
| Type: | Bug → Cleanup/optimization |
Back to the original state of play after changes in #28478.
Actually, not quite the original state of play. allow_thread_sharing was removed in #30171 in favor of a counter pattern which obviates the reported bug here. I still think Chris's suggestion has value, but it's more of a cleanup opportunity, if I'm not mistaken.
Having touched this a couple times I ought to just finish it off.
comment:10 by , 3 years ago
| Patch needs improvement: | set |
|---|
comment:11 by , 3 years ago
| Patch needs improvement: | unset |
|---|
comment:12 by , 3 years ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
This merged pull request removed calling
tearDownClass()twice (because it blocked the resolution of that issue), so I'm retitling this ticket to reflect the updated situation.