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 )
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 , 5 years ago
Description: | modified (diff) |
---|
comment:2 by , 5 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:4 by , 5 years ago
Owner: | set to |
---|
comment:5 by , 4 years ago
Resolution: | → wontfix |
---|---|
Status: | assigned → closed |
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.
+1 to more precise testing!