Opened 8 years ago

Closed 2 years ago

Last modified 9 months ago

#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 Tim Graham, 8 years ago

Component: UncategorizedTesting framework
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

comment:2 by Chris Jerdonek, 8 years ago

Owner: changed from nobody to Chris Jerdonek
Status: newassigned

comment:3 by Chris Jerdonek, 8 years ago

Summary: LiveServerPort.test_port_bind() sets allow_thread_sharing to False before all server threads are doneRefactor LiveServerPort.test_port_bind() not to call setUpClass() twice

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.

comment:4 by Mariusz Felisiak, 3 years ago

Owner: Chris Jerdonek removed
Status: assignednew

in reply to:  3 comment:5 by Jacob Walls, 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 Chris Jerdonek, 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 Jacob Walls, 3 years ago

Summary: Refactor LiveServerPort.test_port_bind() not to call setUpClass() twiceLiveServerPort 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 Jacob Walls, 2 years ago

Owner: set to Jacob Walls
Status: newassigned
Summary: LiveServerPort test methods set allow_thread_sharing to False before all server threads are doneRefactor LiveServerPort tests to not make extra calls to setUpClass() and tearDownClass()
Type: BugCleanup/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:9 by Jacob Walls, 2 years ago

Has patch: set

comment:10 by Jacob Walls, 2 years ago

Patch needs improvement: set

comment:11 by Jacob Walls, 2 years ago

Patch needs improvement: unset

comment:12 by Mariusz Felisiak, 2 years ago

Triage Stage: AcceptedReady for checkin

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In def09bf:

Fixed #27079 -- Avoided multiple setUpClass()/tearDownClass() calls in LiveServerTestCase tests.

comment:14 by Mariusz Felisiak <felisiak.mariusz@…>, 14 months ago

In 534895f:

[4.0.x] Fixed thread termination in servers.tests.LiveServerPort on Python < 3.10.9.

TestCase.doClassCleanups() cannot be called on Python < 3.10.9 because
setUpClass()/tearDownClass() are called multiple times in
LiveServerTestCase tests (refs #27079).

comment:15 by GitHub <noreply@…>, 9 months ago

In d6e9ec4:

Refs #27079 -- Used addClassCleanup() in SeleniumTestCase.

Regression in def09bf4126d4886413adf7388882eca8e32576b.

Note: See TracTickets for help on using tickets.
Back to Top