Opened 5 years ago

Closed 5 years ago

Last modified 16 months ago

#4175 task closed fixed (fixed)

Remove trial's setUpClass and tearDownClass features

Reported by: exarkun Owned by:
Priority: normal Milestone:
Component: trial Keywords:
Cc: Branch: branches/remove-class-fixtures-4175
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description (last modified by exarkun)

These two hooks have been deprecated (#2303) since 8.2. There's significant complexity associated with continuing to support them, and in particular making disttrial (#1784) work with them would be more effort than it's worth. We should remove the feature now.

Change History (8)

comment:1 Changed 5 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/remove-class-fixtures-4175

(In [27798]) Branching to 'remove-class-fixtures-4175'

comment:2 Changed 5 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

Done. Lots of nice crufty code deleted. Very enjoyable. Hope I got it all. Build results.

comment:3 Changed 5 years ago by jml

  • Keywords review removed
  • Owner set to exarkun

Wuu! I've been looking forward to this day for a long time. I envy you for being the one to delete these.

Some possible changes:

  1. The NEWS addition is good, but I personally would mention that a lot of crufty code is deleted, and perhaps mention the other bugs that are obsoleted by this change. Your discretion.
  2. AIUI, we require a blank line after docstrings. At least, PEP 8 does. Please add such a blank line before test_hashability in test_testcase.
  3. Consider splitting test_hashability into two separate tests.

Only the second one is mandatory, and that only if I've interpreted the coding standards correctly. I'm happy for you to make these changes at your discretion and land the branch without another review roundtrip.

Some other comments:

  • Thanks for adding the docstring to the test_results test for skipping.
  • Now that we've got rid of tearDownClass, perhaps we can remove the distinction between TestCase._cleanUp and TestCase._classCleanUp. For another ticket though.
  • Similarly, upDownError should probably be destroyed. Will that let us delete BrokenTestCaseWarning, I wonder.

comment:4 Changed 5 years ago by exarkun

(In [27804]) Split part of test_hashability off into test_equality

refs #4175

comment:5 Changed 5 years ago by exarkun

Thanks for the review!

I lean towards leaving the news entry as it is now because it is describing the user-facing changing adequately. I feel like discussion of implementation issues (despite being pretty great, deleting ~600 lines of pretty gnarly code) don't really belong. Since the other two tickets were closed as wontfix, it also feels out of place to mention them.

I followed up on the PEP 8 thing, and we discussed it on IRC, leading to the revelation that PEP 8 actually says something different, stupid, and wrong about (modern) emacs' fill-paragraph. So, leaving the docstring as-is.

I did split test_hashability in two, though. Good call.

Getting rid of upDownError sounds pretty cool too. As long as it doesn't get in my way for #1784, I'll leave that removal for you. :)

comment:6 Changed 5 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(In [27805]) Merge remove-class-fixtures-4175

Author: exarkun
Reviewer: jml
Fixes: #4175

Remove trial's setUpClass and tearDownClass features (deprecated since 8.2), and
all of the (~600 lines of) crufty code previously supporing them.

comment:7 Changed 3 years ago by <automation>

  • Owner exarkun deleted

comment:8 Changed 16 months ago by exarkun

  • Description modified (diff)
Note: See TracTickets for help on using tickets.