Opened 10 years ago

Closed 10 years ago

Last modified 6 years ago

#4175 task closed fixed (fixed)

Remove trial's setUpClass and tearDownClass features

Reported by: Jean-Paul Calderone Owned by:
Priority: normal Milestone:
Component: trial Keywords:
Cc: Branch: branches/remove-class-fixtures-4175
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description (last modified by Jean-Paul Calderone)

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 10 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/remove-class-fixtures-4175

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

comment:2 Changed 10 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted

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

comment:3 Changed 10 years ago by Jonathan Lange

Keywords: review removed
Owner: set to Jean-Paul Calderone

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 10 years ago by Jean-Paul Calderone

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

refs #4175

comment:5 Changed 10 years ago by Jean-Paul Calderone

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 10 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(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 8 years ago by <automation>

Owner: Jean-Paul Calderone deleted

comment:8 Changed 6 years ago by Jean-Paul Calderone

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