Fix `skipTest` inside `subTest` #169

ydshieh · 2024-11-18T13:45:58Z

Currently, the reporting has issues in the cases where skipTest is used inside subTest. For example

import unittest

class T(unittest.TestCase):
    def test_foo(self):
        for i in range(10):
            with self.subTest("custom message", i=i):
                if i < 4:
                    self.skipTest(f"skip {i}")
                assert i < 4

outputs

tests/test_foo.py::T::test_foo [custom message] (i=4) SUBSKIP (skip 0)
tests/test_foo.py::T::test_foo [custom message] (i=5) SUBSKIP (skip 1)
tests/test_foo.py::T::test_foo [custom message] (i=6) SUBSKIP (skip 2)
tests/test_foo.py::T::test_foo [custom message] (i=7) SUBSKIP (skip 3)
tests/test_foo.py::T::test_foo [custom message] (i=8) SUBFAIL
tests/test_foo.py::T::test_foo [custom message] (i=9) SUBFAIL
tests/test_foo.py::T::test_foo PASSED
...
=============== short test summary info ===============
[custom message] (i=8) SUBFAIL tests/test_foo.py::T::test_foo - AssertionError: assert 8 < 4
[custom message] (i=9) SUBFAIL tests/test_foo.py::T::test_foo - AssertionError: assert 9 < 4
=============== 2 failed, 1 passed, 4 skipped in 0.15s  ===============

which is obviously wrong.

This PR fix the above issue. The new output is

tests/test_foo.py::T::test_foo [custom message] (i=0) SUBSKIP (skip 0)
tests/test_foo.py::T::test_foo [custom message] (i=1) SUBSKIP (skip 1)
tests/test_foo.py::T::test_foo [custom message] (i=2) SUBSKIP (skip 2)
tests/test_foo.py::T::test_foo [custom message] (i=3) SUBSKIP (skip 3)
tests/test_foo.py::T::test_foo [custom message] (i=4) SUBFAIL
tests/test_foo.py::T::test_foo [custom message] (i=5) SUBFAIL
tests/test_foo.py::T::test_foo [custom message] (i=6) SUBFAIL
tests/test_foo.py::T::test_foo [custom message] (i=7) SUBFAIL
tests/test_foo.py::T::test_foo [custom message] (i=8) SUBFAIL
tests/test_foo.py::T::test_foo [custom message] (i=9) SUBFAIL
tests/test_foo.py::T::test_foo PASSED                                                                                                                                                                        
...
=============== short test summary info ===============
 [custom message] (i=4) SUBFAIL tests/test_foo.py::T::test_foo - AssertionError: assert 4 < 4
[custom message] (i=5) SUBFAIL tests/test_foo.py::T::test_foo - AssertionError: assert 5 < 4
[custom message] (i=6) SUBFAIL tests/test_foo.py::T::test_foo - AssertionError: assert 6 < 4
[custom message] (i=7) SUBFAIL tests/test_foo.py::T::test_foo - AssertionError: assert 7 < 4
[custom message] (i=8) SUBFAIL tests/test_foo.py::T::test_foo - AssertionError: assert 8 < 4
[custom message] (i=9) SUBFAIL tests/test_foo.py::T::test_foo - AssertionError: assert 9 < 4
=============== 6 failed, 1 passed, 4 skipped in 0.20s  ===============

for more information, see https://pre-commit.ci

…into fix_skip

ydshieh · 2024-11-18T17:52:53Z

src/pytest_subtests/plugin.py

+    TestCaseFunction._originaladdSkip = copy.copy(TestCaseFunction.addSkip)  # type: ignore[attr-defined]
+    TestCaseFunction.addSkip = _addSkip  # type: ignore[method-assign]


The _addSkip in unittest.case doesn't contain logic to call addSubTest for outcome.skipped (unlike _feedErrorsToResult), and it turns out this causes the reporting issue described in the PR.

Unlike addSubTest (where original TestCaseFunction doesn't have it), addSkip is defined TestCaseFunction, and here we save the original implementation and patch TestCaseFunction.addSkip to a newly defined _addSkip that allows us to call addSubTest.

The _originaladdSkip allows us to restore to it in pytest_unconfigure as well as it turns out necessary to call it too inside the newly defined _addSkip.

dvrogozh · 2024-11-18T17:53:25Z

tests/test_foo.py::T::test_foo PASSED

Why the whole test is marked as passed if all subtests either failed or skipped? or we need to consider that there might be a testing code in the test out of subtest code? If so, I wish for documentation example for that with description.

I also wonder how python unittest reports such failures? is pytest-subtests behavior aligned?

src/pytest_subtests/plugin.py

ydshieh · 2024-11-18T18:34:59Z

tests/test_foo.py::T::test_foo PASSED

Why the whole test is marked as passed if all subtests either failed or skipped? or we need to consider that there might be a testing code in the test out of subtest code? If so, I wish for documentation example for that with description.

I also wonder how python unittest reports such failures? is pytest-subtests behavior aligned?

I am not 100% sure though as I don't dive into the whole unittest / pytest-subtests, but

there might be a testing code in the test out of subtest code?`

is what I observed (even on the current main branch).

I also wonder how python unittest reports such failures? is pytest-subtests behavior aligned?

~~I will check.~~

well, unittest gives

FAILED (failures=6, skipped=4)

ydshieh · 2024-11-18T18:59:24Z

src/pytest_subtests/plugin.py

@@ -98,6 +100,24 @@ def _from_test_report(cls, test_report: TestReport) -> SubTestReport:
        return super()._from_json(test_report._to_json())


+def _addSkip(self: TestCaseFunction, testcase: TestCase, reason: str) -> None:
+    if isinstance(testcase, _SubTest):
+        self._originaladdSkip(testcase, reason)  # type: ignore[attr-defined]


It seems we don't really need call self._originaladdSkip for subtest skips, i.e. the following also works

try: raise pytest.skip.Exception(reason, _use_item_location=True) except skip.Exception: exc_info = sys.exc_info() self.addSubTest(testcase.test_case, testcase, exc_info)

for more information, see https://pre-commit.ci

ydshieh · 2024-11-19T09:15:26Z

cc @nicoddemus

nicoddemus

Hi @ydshieh thanks for the PR

Other than my comments, we need a test to ensure this does not regress in the future.

src/pytest_subtests/plugin.py

for more information, see https://pre-commit.ci

ydshieh · 2024-11-20T19:59:48Z

Hi @ydshieh thanks for the PR

Other than my comments, we need a test to ensure this does not regress in the future.

Good point! I will add some tests

for more information, see https://pre-commit.ci

ydshieh · 2024-11-21T15:56:27Z

src/pytest_subtests/plugin.py

+    else:
+        # For python < 3.11: the non-subtest skips have to be added by `_originaladdSkip` only after all subtest
+        # failures are processed by `_addSubTest`.
+        if sys.version_info < (3, 11):


python unittest has some changes (since 3.11) which leads to this if/else here

…into fix_skip

for more information, see https://pre-commit.ci

ydshieh and others added 10 commits November 18, 2024 14:28

fix

c62d3ae

fix more: non subtest skip cases

c5c02fa

[pre-commit.ci] auto fixes from pre-commit.com hooks

a439226

for more information, see https://pre-commit.ci

fix: # type: ignore[method-assign]

866c990

Merge branch 'fix_skip' of https://github.com/ydshieh/pytest-subtests …

590624d

…into fix_skip

fix more: # type: ignore[attr-defined]

a737260

fix more: # type: ignore[attr-defined]

2e38614

fix more: # type: ignore[attr-defined]

d7f0fa5

fix more: # type: ignore[attr-defined]

aa4a17b

fix more: # type: ignore[attr-defined]

d6e9e5e

ydshieh mentioned this pull request Nov 18, 2024

Issues counting passing rates on tests which use subTest() huggingface/transformers#34755

Open

ydshieh commented Nov 18, 2024

View reviewed changes

src/pytest_subtests/plugin.py Outdated Show resolved Hide resolved

ydshieh commented Nov 18, 2024

View reviewed changes

src/pytest_subtests/plugin.py Outdated Show resolved Hide resolved

ydshieh commented Nov 18, 2024

View reviewed changes

ydshieh and others added 2 commits November 18, 2024 21:05

fix None exec_info in subtest error

a99ea3b

[pre-commit.ci] auto fixes from pre-commit.com hooks

07ef703

for more information, see https://pre-commit.ci

This was referenced Nov 19, 2024

[DO NOT MERGE] Add pytest-subtests to the dependencies huggingface/transformers#34753

Draft

Fix check_training_gradient_checkpointing huggingface/transformers#34806

Merged

ydshieh marked this pull request as ready for review November 20, 2024 09:16

nicoddemus requested changes Nov 20, 2024

View reviewed changes

src/pytest_subtests/plugin.py Outdated Show resolved Hide resolved

src/pytest_subtests/plugin.py Outdated Show resolved Hide resolved

src/pytest_subtests/plugin.py Outdated Show resolved Hide resolved

src/pytest_subtests/plugin.py Outdated Show resolved Hide resolved

ydshieh and others added 4 commits November 20, 2024 20:28

remove copy

67993fe

style

23929b5

[pre-commit.ci] auto fixes from pre-commit.com hooks

7461dd6

for more information, see https://pre-commit.ci

comment

92e427c

fix python versions

a9c852e

ydshieh and others added 6 commits November 21, 2024 13:41

fix recursion

f9c0fba

fix recursion

aecfe7a

Add first test

87fcb67

Add first test

01c2b12

[pre-commit.ci] auto fixes from pre-commit.com hooks

450e81e

for more information, see https://pre-commit.ci

add more tests

fd94cb9

ydshieh requested a review from nicoddemus November 21, 2024 15:55

ydshieh commented Nov 21, 2024

View reviewed changes

ydshieh and others added 3 commits November 21, 2024 21:20

add more tests

f75438d

Merge branch 'fix_skip' of https://github.com/ydshieh/pytest-subtests …

d14628b

…into fix_skip

[pre-commit.ci] auto fixes from pre-commit.com hooks

15612c0

for more information, see https://pre-commit.ci

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `skipTest` inside `subTest` #169

Fix `skipTest` inside `subTest` #169

ydshieh commented Nov 18, 2024

ydshieh Nov 18, 2024

dvrogozh commented Nov 18, 2024

ydshieh commented Nov 18, 2024 •

edited

Loading

ydshieh Nov 18, 2024

ydshieh commented Nov 19, 2024

nicoddemus left a comment

ydshieh commented Nov 20, 2024

ydshieh Nov 21, 2024 •

edited

Loading

		TestCaseFunction._originaladdSkip = copy.copy(TestCaseFunction.addSkip) # type: ignore[attr-defined]
		TestCaseFunction.addSkip = _addSkip # type: ignore[method-assign]

Fix skipTest inside subTest #169

Are you sure you want to change the base?

Fix skipTest inside subTest #169

Conversation

ydshieh commented Nov 18, 2024

ydshieh Nov 18, 2024

Choose a reason for hiding this comment

dvrogozh commented Nov 18, 2024

ydshieh commented Nov 18, 2024 • edited Loading

ydshieh Nov 18, 2024

Choose a reason for hiding this comment

ydshieh commented Nov 19, 2024

nicoddemus left a comment

Choose a reason for hiding this comment

ydshieh commented Nov 20, 2024

ydshieh Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Fix `skipTest` inside `subTest` #169

Fix `skipTest` inside `subTest` #169

ydshieh commented Nov 18, 2024 •

edited

Loading

ydshieh Nov 21, 2024 •

edited

Loading