镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

3.12 starts to return an error when all tests in a test file are skipped #113661

Closed

Description

@fishy

Bug report

Bug description:

See https://github.com/apache/thrift/blob/5cf71b2beec3c67a4c8452ddabbbc6ae43fff16f/lib/py/test/test_sslsocket.py for a full example, we skipped all tests in that file via:

@unittest.skip("failing SSL test to be fixed in subsequent pull request")
class TSSLSocketTest(unittest.TestCase):
    ...

On python 3.11 this is fine with:

/usr/bin/python3 test/test_sslsocket.py
sssssssssss
----------------------------------------------------------------------
Ran 11 tests in 0.000s

OK (skipped=11)

But on python 3.12, this starts to become an error:

/opt/hostedtoolcache/Python/3.12.1/x64/bin/python test/test_sslsocket.py
WARNING:thrift.transport.sslcompat:using legacy validation callback
sssssssssss
----------------------------------------------------------------------
Ran 0 tests in 0.000s

NO TESTS RAN (skipped=11)
make[1]: *** [Makefile:645: py3-test] Error 5

I have to add a not-skipped dummy test to work around this (https://github.com/apache/thrift/pull/2914/files)

Is this an intentional change? I found it weird that we consider a skipped test as a failure.

CPython versions tested on:

3.12

Operating systems tested on:

Linux

Linked PRs

Activity

  1. added
    type-bugAn unexpected behavior, bug, or error
    on Jan 2, 2024
  2. terryjreedy commented on Jan 2, 2024

    @terryjreedy
    Member

    The change is quite intentional. See #98903.

    @vstinner The issue PR #98904 added 4 new exit codes -- 2, 3, 4, and 130 -- to libregtest.main, since moved to libregtest.results. As far as I can detect, they are not mentioned in What's New 3.12. I think they should be. But what needs to be said? Just the new error situations, or the numbers also?

    To add confusion, the unittest.main doc says that all-skipped has exit code 5, whereas you added 4. The other additions are not mentioned and there is no new-in 3.12 notice. Are these errors and omissions?

    #106584 quoted 5 from the doc. I changed this to 4 from the code, but then realized that unittest is not regrtest so wonder if that change is correct. Is does not matter in a sense because neither constant appears in the code example or the patch.

  3. fishy commented on Jan 2, 2024

    @fishy
    Author

    I would argue that the original issue #98903 is different from the end result in 3.12.

    I can understand the rationale to fail a test if "no test is found", with the intention to catch typos. But "all tests are intentionally skipped" is quite different from "there are not really any tests in the file because of typos", and it should not be too hard to distinguish between the 2 cases?

    The original bug report also says:

    it displays "NO TEST RUN" but the exit code is a success

    Which reinforces my belief that this end result (that it fails when all tests were skipped) is unintentional, as that used to display "OK (skipped=N)" in 3.11.

  4. fishy commented on Jan 2, 2024

    @fishy
    Author

    Also looking into the PR, its definition of no_tests_run is:

        def no_tests_run(self):
            return not any((self.good, self.bad, self.skipped, self.interrupted,
                            self.environment_changed))

    Which seems to suggest that when there were skipped tests, it's supposed to return False? So failing a test for the reason of NO TESTS RAN with skipped tests seem to be a bug?

  5. zware commented on Jan 3, 2024

    @zware
    Member

    The change is quite intentional. See #98903.

    @vstinner The issue PR #98904 added 4 new exit codes -- 2, 3, 4, and 130 -- to libregtest.main, since moved to libregtest.results. As far as I can detect, they are not mentioned in What's New 3.12. I think they should be. But what needs to be said? Just the new error situations, or the numbers also?

    To add confusion, the unittest.main doc says that all-skipped has exit code 5, whereas you added 4. The other additions are not mentioned and there is no new-in 3.12 notice. Are these errors and omissions?

    #98903 and its related changes are about the CPython test suite and its regrtest runner, it is unrelated to unittest.

    #106584 quoted 5 from the doc. I changed this to 4 from the code, but then realized that unittest is not regrtest so wonder if that change is correct.

    It was not; I've reverted the change to the comment in #106584.

    All that said, @terryjreedy is correct that the behavior here is as desired: generally speaking, if all of your tests are skipped, you aren't actually testing what you think you are and your test runner should complain about it. See #62432/#102051 for the original discussion. We should have added a whatsnew entry about it at the time, though.

  6. carljm commented on Jan 3, 2024

    @carljm
    Member

    @zware I don't see any discussion of the "nonzero tests found but all skipped" situation in any of the threads you linked.

    I don't find the argument "if all tests are skipped you aren't testing what you think you are" to be convincing. I can imagine many reasons why all tests found in a particular test run might be skipped, which should not be considered an error. I can't really imagine when I'd ever want that situation to be reported as an error. The whole point of skipping a test is that it shouldn't cause the run to fail.

    I find "no tests found is an error" a useful behavior, and quite different.

    Unless someone has a link to previous discussion of the "all skipped" scenario that makes it clear this was intentional and has good rationale, I think the behavior reported here is both a bug and a regression that should be fixed and backported.

  7. AlexWaygood commented on Jan 4, 2024

    @AlexWaygood
    Member

    Not proposing that we close this as a duplicate (as this issue has more discussion), but is this the same issue that was reported in #113267?

  8. carljm commented on Jan 4, 2024

    @carljm
    Member

    It seems plausible that the cause could the same / related (I haven't dug into the implementation details here), but the symptoms are definitely distinct, so I wouldn't consider them the same issue.

  9. vstinner commented on Jan 8, 2024

    @vstinner
    Member

    I'm using "No tests ran" and exit code 4 to detect typos in the -m parameter. For example, ./python -m test test_os -m test_acces: test_acces (missing trailing S) instead of test_access.

    Maybe we should not consider that "no tests were run" if at least one test was skipped?

    For example, replace:

        def no_tests_run(self):
            return not any((self.good, self.bad, self.skipped, self.interrupted,
                            self.environment_changed))

    with:

        def no_tests_run(self):
            return not any((self.good, self.bad, self.skipped, self.interrupted,
                            self.environment_changed)) and (not self.skipped)
  10. vstinner commented on Jan 8, 2024

    @vstinner
    Member

    Maybe we should not consider that "no tests were run" if at least one test was skipped?

    We can also use a different heuristic using "are tests filtered?" condition. For example, are -m and/or -i command line options used? See RunTests.match_tests member.

    Note: RunTests.match_tests_dict is different, it's used by the --rerun feature.

  11. zware commented on Jan 8, 2024

    @zware
    Member

    To be clear, this issue has nothing to do with the CPython test suite (aka libregrtest, aka python -m test), it's about unittest (python -m unittest, unittest.main()) only.

    @zware I don't see any discussion of the "nonzero tests found but all skipped" situation in any of the threads you linked.

    Sorry, that's #106584, which to be fair has next to no discussion. cc author @EliseevEgor and committer @hugovk.

  12. vstinner commented on Jan 8, 2024

    @vstinner
    Member

    To be clear, this issue has nothing to do with the CPython test suite

    Oh. Now I'm confused.

    @fishy: Would you mind to write a short reproducer of your issue?

    See https://github.com/apache/thrift/blob/5cf71b2beec3c67a4c8452ddabbbc6ae43fff16f/lib/py/test/test_sslsocket.py for a full example, we skipped all tests in that file via (...)

    This example uses a third party module, so it's not easy to run without having to install Apache Thrift.

  13. hugovk commented on Jan 8, 2024

    @hugovk
    Member

    Sorry, that's #106584, which to be fair has next to no discussion. cc author @EliseevEgor and committer @hugovk.

    cc committer @ambv (I closed the issue two months later).

  14. zware commented on Jan 8, 2024

    @zware
    Member

    🤦‍♂️ Sorry for the noise, Hugo.

  15. 7 remaining items

  16. added a commit that references this issue on Jan 9, 2024
  17. added a commit that references this issue on Jan 9, 2024
  18. added a commit that references this issue on Jan 9, 2024
  19. self-assigned this
    on Jan 9, 2024
  20. gpshead commented on Jan 9, 2024

    @gpshead
    Member

    thanks for the report and PR!

  21. added a commit that references this issue on Jan 22, 2024
  22. added a commit that references this issue on Feb 11, 2024
  23. added a commit that references this issue on Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

3.12only security fixes3.13only security fixesstdlibStandard Library Python modules in the Lib/ directorytestsTests in the Lib/test dirtype-bugAn unexpected behavior, bug, or error

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions