Repository navigation
TST: should we adopt __tracebackhide__=True in assert_* test helper functions ? #16071
Description
numpy.testing.assert_* functions set __traceback__ = True, which changes pytest's behavior so that the internals of these functions is not shown in errors. This is particularly useful when invoking pytest with --pdb, which opens a debugger when (and where) an exception is raised: because the internals of assert_* functions are excluded from the traceback, the location associated with the error is that of the callsite, which is usually the body of a test function.
Let me illustrate what I mean with a simplified example
# t.py
import numpy as np
def assert_equal_len_v1(x, y):
assert len(x) == len(y)
def assert_equal_len_v2(x, y):
__tracebackhide__ = True
assert len(x) == len(y)
def test_v1():
a = np.arange(2)
b = np.arange(3)
assert_equal_len_v1(a, b)
def test_v2():
a = np.arange(2)
b = np.arange(3)
assert_equal_len_v2(a, b)run with pytest --pdb t.py and see where the debugger is opened: test_v1 will open in within assert_equal_v1's body, while test_v2 opens it in its own body, which is arguably more useful since we can inspect the whole local scope, instead of just the couple variables that were passed (under different names !) to a test helper.
I propose leveraging this mechanism in all assert_* helper functions (none of which are public API as they live in test code). Any counter arguments ?
Activity
neutrinoceros commentedon Feb 19, 2024
I forgot to mention that pytest also has a flag to ignore these markers when needed:
--full-tracepllim commentedon Feb 19, 2024
That would touch a lot of code. I am not sure if the cost outweighs the benefit here. Over 10 years without this and we still managed to debug.
eerovaher commentedon Feb 20, 2024
Just adding one line to some of our test helper functions would provide a lot of useful functionality, and not just to us but to everybody that might be using those functions in their development work. I don't know what the remark about touching a lot of code in #16071 (comment) is referring to.
pllim commentedon Feb 20, 2024
Maybe I am not completely understanding the ask. I thought you want to insert that line wherever there is a bare
assertcheck.xref #16069 (comment)
eerovaher commentedon Feb 20, 2024
We would only modify test helper functions like #16069 edited
astropy.tests.helper.assert_quantity_allclose(). There's no need to touch any of the tests (i.e.test_*()).neutrinoceros commentedon Feb 20, 2024
Indeed, I'm only proposing to add this line to every function named
assert_<something>, which according to this simple command would require only 34 lines.(but the whole idea is to make their call site behave as bare
assertstatements do in pytest !)pllim commentedon Feb 20, 2024
If it is only 34 lines, then doesn't hurt to just open a PR and see. Then, I don't understand about the comment in #16069 (comment) that said this would touch too many lines.
neutrinoceros commentedon Feb 20, 2024
It's just that I naively opened my PR thinking there would only be one function to touch, so >10 seemed like "much" in comparison 😅
I'll open a new one with the +34 patch !
neutrinoceros commentedon Feb 20, 2024
While doing this I found there actually is a precedent for using this !
astropy/astropy/time/tests/test_precision.py
Line 74 in 2fd5783
from #10373
mhvk commentedon Feb 20, 2024
Yes, a very good idea! I am always annoyed that in the debugger I first have to go up a few steps before getting to the point where I can see what actually went wrong.
jdavies-st commentedon Mar 13, 2024
This is a very good idea. We already do this over in
jwstfor all our diff and assertion helpers. It filters out the garbage internals of the assertion helper and gives you the real traceback of what is failing in the test. This gets a big 👍 from me.