Opened 5 years ago

Closed 4 years ago

#30866 closed Cleanup/optimization (wontfix)

Refactor Error Reporting tests in tests/view_tests/tests/test_debug.py

Reported by: Carlton Gibson Owned by: Carlton Gibson
Component: Error reporting Version: 2.2
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Carlton Gibson)

The tests in tests/view_tests/tests/test_debug.py are all (for want of a better phrase) integration level — they test whether particular values appear in the final output of either HTML, Plain Text, or either version of the email that's sent to admins.

The trouble with this is that there's lot's of duplicate testing. As is inevitable, the tests are subtly out of sync. There are gaps in coverage, for instance in testing whether settings overrides actually work. And most, for me, looking at resolving the outstanding Error Reporting issues at the moment, it's hard to keep in mind exactly what tests should go where.

As such, I'd like to refactor the tests. I'm just about to take that on, but I wanted to confirm that it would be OK before doing so. Coverage will be maintained. The tests will be clearer and just as effective, if not more so, and much easier to comprehend/maintain.

General strategy, having looked at it:

All methods go via ExceptionReporter.get_traceback_data(), so I want to reduce all the Did the filtering work? tests down to that (or a single output format if testing the technical_500 view).

Then, the HTML or Plain text formats, use ExceptionReporter.get_traceback_data(). They in turn are used by the AdminEmailHandler. So not to loose the assurance that the final output is safe, I'd like to use mocks to assert that get_traceback_data() was actually employed. (Not usually a fan of mocking, but in this case it's right.)

Additional tests can then focus on format specific questions, such as whether the correct format is used for Ajax requests, rather than repeating the filtering logic multiple times.

As I say, primed to go but, wanting to confirm and record the intention, because it's a change in strategy.

Change History (5)

comment:1 by Carlton Gibson, 5 years ago

Description: modified (diff)

comment:2 by Claude Paroz, 5 years ago

Triage Stage: UnreviewedAccepted

+1 to more precise testing!

comment:3 by Carlton Gibson, 5 years ago

OK, thanks Claude! (Got my weekend project then. 🙂)

Last edited 5 years ago by Carlton Gibson (previous) (diff)

comment:4 by Carlton Gibson, 5 years ago

Owner: set to Carlton Gibson

comment:5 by Carlton Gibson, 4 years ago

Resolution: wontfix
Status: assignedclosed

I made a start on this, but got sidelined with other things. Given the improvements to error reporting for 3.1, and the small number of open issues on this component, I don't want to give it the extra time to address this now. (Not currently seeing the ROI.)

I or someone else could come back to it in the future, but pending a PR I'm going to resolve as wontfix.

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