#21509 closed Cleanup/optimization (fixed)

Remove explicit catching of SystemExit or Keboardinterrupt

Reported by: KJ Owned by: KJ
Component: Uncategorized Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

In assertXMLEqual and assertXMLNotEqual methods, test result is determined as following:

try:
    result = compare_xml(xml1, xml2)
except Exception as e:
    standardMsg = 'First or second argument is not valid XML\n%s' % e
    self.fail(self._formatMessage(msg, standardMsg))

Because of that, KeyboardInterrupt and SystemExit raised during XML comparison are suppressed.

Change History (6)

comment:1 Changed 22 months ago by KJ

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I’ve created a pull request: https://github.com/django/django/pull/1992

comment:2 Changed 22 months ago by bmispelon

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

Hi,

KeyboardInterrupt and SystemExit are not caught by an except Exception clause (that's because they inherit from BaseException):

>>> issubclass(SystemExit, Exception)
False
>>> issubclassKeyboardInterrupt, Exception)
False

The code change you proposed would have no effect.

comment:3 Changed 22 months ago by KJ

  • Resolution invalid deleted
  • Status changed from closed to new

Indeed, I was misleaded, by this legacy code: https://github.com/django/django/blob/3c10d1e64faeb67b41e7aa501b21252e357a4564/django/test/testcases.py#L178 . Since we don’t support Python 2.4 and lower anymore, such parts of code can be removed, so I’m reopening the ticket.

comment:4 Changed 22 months ago by bmispelon

  • Component changed from Testing framework to Uncategorized
  • Has patch unset
  • Summary changed from XML assertions suppress KeyboardInterrupt and SystemExit to Remove explicit catching of SystemExit or Keboardinterrupt
  • Triage Stage changed from Unreviewed to Accepted

Indeed, that is not necessary anymore. Good catch.

A cleanup of the whole codebase to remove this outdated pattern would be nice.

I'm accepting the ticket on this basis (and changing the description accordingly).

comment:5 Changed 22 months ago by KJ

  • Has patch set

Ok, I’ve opened another pull request: https://github.com/django/django/pull/1993 . I’ve grepped the whole codebase, but found this pattern only in one file.

comment:6 Changed 22 months ago by Baptiste Mispelon <bmispelon@…>

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

In 7a0a3a64a827480c737060bcd5af9af25c3b3c15:

Fixed #21509 -- Removed dead exception catching code.

Since Python 2.5, KeyboardInterrupt and SystemExit are not subclasses of
Exception, so explicitly reraising them before the “except Exception” clause
is not necessary anymore.

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