Repository navigation
Improve subtests clean up #1719
Description
Description of issue or feature request:
In tests we use the home-made decorator @utils.run_sub_tests_with_dataset when we need to execute the same test case consecutively for many datasets. The decorator works perfectly fine when no clean up is needed after each sub test but in the cases where clean up code needs to be executed, the whole sub test needs to be included in a "try - finally" block in order to run the clean up code even if a sub test fails.
Expected behavior:
Improve the decorator by possibly including the clean up logic in it to avoid the big "try" blocks in the test code.
See #1689 (comment)
Reacted by Jussi Kukkonen
Activity
jku commentedon Dec 13, 2021
another related thing is
case_namethat is set in the decorator now: it's quite useful but leads to multiple workarounds with both pylint and mypy correctly complaining about it. I think if the decorator is part of a class (something derived from unittest.TestCase) then this would probably be a fixable situation.abs007 commentedon May 26, 2022
@jku I went through your comment and thought of this.
Have a
SubTestCase(unittest.TestCase)class which would use an__init__()method to initialize the original function (the one that uses the decorator) and tie it to the class instance and then use a__call__()method to allow for calling the class.__call__()would essentially contain the decorator code andcls.setup_subtest(),cls.teardown_subtest()(which I believe is the cleanup code) would also be managed inside this class.I would like your thoughts on this.
MVrachev commentedon Jun 9, 2022
Sounds like a reasonable idea.
By implementing the
__call__method you will call the decorator and thecls.setup_subtest()logic, but when and how do you call thecls.teardown_subtest()logic?As I understand it we will call the
__call__method with each test case we are given and that's why I am wondering how do you imagine fitting theteardown_subtestlogic in that?abs007 commentedon Jun 11, 2022
Would something like this work? (please excuse me if I'm not making sense logically)
What I'm thinking is that
__call__()won't be called multiple times now since we're putting the decorator code in thewrapper()func.MVrachev commentedon Jun 20, 2022
It seems to make sense. You call the decorator and then the setup function.
I haven't tested what is first now - the decorator or the
setUpfunction.Can you choose a testing function using the decorator and link to an example how what it would look like?
It will be great if you push an example branch with this change in your fork, so we can have a look closer.
Then it would be easier to validate that idea.
abs007 commentedon Jul 2, 2022
Sorry, it looks like I got things mixed up. I thought @jku meant a decorator class by his comment (A class mimicking a decorator function). Some things that I'd like to ask first.
My question is that do we need to pass
setup_subtest()inside thesetUpClass()method (I'm guessing this is where he meant to put it) since we seem to use it after calling the decorator like in here. I'm saying this since setUpClass() will pass only once before all the test cases are run. Also, willsetUpClass()run in the first place if we don't have any test functions inside the class? (which I don't think we're supposed to have since we only have our decorator logic)mhb8898 commentedon May 14, 2026
Hi! I'd like to work on this issue. This would be my first contribution to the project and I'm excited to start helping out.
My proposed fix is to update the
run_sub_tests_with_datasetdecorator intests/utils.pyto automatically callteardown_subtest()in afinallyblock after each subtest case runs:This removes the need for manual
try/finallyblocks in the test methods. Thehasattrcheck keeps it backward compatible with test classes that don't defineteardown_subtest.This would clean up the boilerplate in
test_updater_delegation_graphs.py(5 methods) andtest_updater_consistent_snapshot.py(3 methods). All existing tests pass after the change.I also read @jku's suggestion about wrapping this into a
SubTestCaseclass withsetup_subtest/teardown_subtestas class methods. I think that's a great idea for a more complete solution, but I wanted to start smaller — this decorator change already removes all thetry/finallyboilerplate and improves the code meaningfully. We can always build on top of it later.Happy to open a PR if this approach looks good to you!
Pratik060921 commentedon Aug 12, 2026
Hi! I'd like to work on this issue. This would be my first contribution to the project and I'm excited to start helping out.
My proposed fix is to update the run_sub_tests_with_dataset decorator in tests/utils.py to automatically call teardown_subtest() in a finally block after each subtest case runs:
def wrapper(test_cls: unittest.TestCase) -> None:
for case, data in dataset.items():
with test_cls.subTest(case=case):
test_cls.case_name = case.replace(" ", "_")
try:
function(test_cls, data)
finally:
if hasattr(test_cls, "teardown_subtest"):
test_cls.teardown_subtest()
This removes the need for manual try/finally blocks in the test methods. The hasattr check keeps it backward compatible with test classes that don't define teardown_subtest.
This would clean up the boilerplate in test_updater_delegation_graphs.py (5 methods) and test_updater_consistent_snapshot.py (3 methods). All existing tests pass after the change.
Happy to open a PR if this approach looks good to you!