Skip to content

Conversation

mhsmith
Copy link
Member

@mhsmith mhsmith commented Mar 13, 2024

When testing on Android I noticed that test_doctest and test_zipimport_support (which calls test_doctest) were skipping entire modules because of missing subprocess support, even though only one test method actually required subprocesses. This PR makes the skip more selective.

@bedevere-app bedevere-app bot added the tests Tests in the Lib/test dir label Mar 13, 2024
@mhsmith
Copy link
Member Author

mhsmith commented Mar 21, 2024

@sobolevn: You've done some work in this area recently; are you able to review this PR?

There's one test failure, but I don't think it's related.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I like the idea, no need to skip the whole module just because one test requires the subprocess module.

Traceback (most recent call last):
...
FileNotFoundError: [Errno ...] ...nosuchfile...
if support.has_subprocess_support:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I don't like is that the diff is huge. Maybe we can do something like

def doctest_requires_subproccess(func):
    if not support.has_subprocess_support:
        func.__doc__ = None  # skip the doctest by setting it to `None`
    return func

@doctest_requires_subprocess
def test_CLI():  # as before, no changes
    ...

I've tried this locally with if True:

» ./python.exe -m test test_doctest -m test_CLI
Using random seed: 4265888589
0:00:00 load avg: 2.23 Run 1 test sequentially
0:00:00 load avg: 2.23 [1/1] test_doctest
test_doctest ran no tests

== Tests result: NO TESTS RAN ==

1 test run no tests:
    test_doctest

Total duration: 59 ms
Total tests: run=0 (filtered)
Total test files: run=1/1 (filtered) run_no_tests=1
Result: NO TESTS RAN

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea; I've generalized it slightly so it can accept any skip condition.

@sobolevn
Copy link
Member

I want to double check our idea, maybe with @serhiy-storchaka ?
I know that you are also interested in doctest module and its tests.

@serhiy-storchaka serhiy-storchaka self-requested a review March 21, 2024 21:19
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one minor problem -- a skipped test just disappears from the output. It is not counted as skipped.

@sobolevn
Copy link
Member

Yes, but I don't think that there's an easy way to fix that. Since the whole module is counted as a single DocTestSuite, we can see how it behaves with other skips (with -OO for example):

» ./python.exe -OO -m test test_doctest            
Using random seed: 1564874428
0:00:00 load avg: 2.01 Run 1 test sequentially
0:00:00 load avg: 2.01 [1/1] test_doctest

== Tests result: SUCCESS ==

1 test OK.

Total duration: 246 ms
Total tests: run=10 skipped=1
Total test files: run=1/1
Result: SUCCESS

Only 1 is reported to be skipped, while in reality all doctests were skipped in this module.

So, not reporting a single doctest is in line with that.

@serhiy-storchaka
Copy link
Member

It is far from ideal, but we can set a docstring with a skipped doctest instead of None.

@mhsmith
Copy link
Member Author

mhsmith commented Mar 27, 2024

In the current state a skipped doctest still isn't counted as skipped by unittest – it just shows as a passed test, even in the verbose log. So I've made a small change to DocTestCase to improve that.

@mhsmith mhsmith removed the skip news label Mar 27, 2024
@sobolevn
Copy link
Member

I think that you are fixing this in the wrong PR :)
Please, create a new issue and a new PR about doctest skip feature.

And you can keep this PR focused on just doctest + subprocess behavior.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Although I agree that the change in DocTestCase deserves a separate issue or at least a separate PR (you can also make it a part of the larger issue #108885). It is larger and more interesting than your initial PR. But anyway, both changes LGTM.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also think that we can add @doctest_skip and @doctest_skip_if decorators to the stdlib. They might be useful for others. This should be a new issue as well :)

@mhsmith
Copy link
Member Author

mhsmith commented Mar 27, 2024

I agree that the change in DocTestCase deserves a separate issue or at least a separate PR

OK, I've split it out into #117297.

I also think that we can add @doctest_skip and @doctest_skip_if decorators to the stdlib. They might be useful for others.

I looked into allowing doctests to use the existing unittest.skip decorators, but that isn't so easy, because by the time we come to run the test, we no longer have a reference to the function whose docstring it came from.

Anyway, I've already got way deeper into this area than I was planning, so I'll leave that to someone else. 😄

@sobolevn
Copy link
Member

than I was planning, so I'll leave that to someone else.

I will take it from here, thank you for this PR! 👍

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Just one comment.

@mhsmith
Copy link
Member Author

mhsmith commented Apr 4, 2024

@sobolevn: Are you able to merge this PR?

@sobolevn
Copy link
Member

sobolevn commented Apr 9, 2024

Yes, sure! Thanks a lot for working on this 👍

@sobolevn sobolevn merged commit 22b25d1 into python:main Apr 9, 2024
@sobolevn
Copy link
Member

sobolevn commented Apr 9, 2024

What do you think about 3.12 backport?

@mhsmith
Copy link
Member Author

mhsmith commented Apr 9, 2024

On 3.12 the only platforms that don't support subprocesses are Emscripten and WASI, so a backport would slightly improve test coverage for them.

However, to actually report the test as skipped would require #117297, which shouldn't be backported because it's a behavior change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants