Opened 3 years ago

Closed 3 years ago

#21509 closed Cleanup/optimization (fixed)

Remove explicit catching of SystemExit or Keboardinterrupt

Reported by: Krzysztof Jurewicz Owned by: Krzysztof Jurewicz
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 3 years ago by Krzysztof Jurewicz

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 3 years ago by Baptiste Mispelon

Resolution: invalid
Status: newclosed

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 3 years ago by Krzysztof Jurewicz

Resolution: invalid
Status: closednew

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 3 years ago by Baptiste Mispelon

Component: Testing frameworkUncategorized
Has patch: unset
Summary: XML assertions suppress KeyboardInterrupt and SystemExitRemove explicit catching of SystemExit or Keboardinterrupt
Triage Stage: UnreviewedAccepted

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 3 years ago by Krzysztof Jurewicz

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 3 years ago by Baptiste Mispelon <bmispelon@…>

Resolution: fixed
Status: newclosed

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