Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-116622: Enable test_doctest on platforms that don't support subprocesses #116758

Merged
merged 7 commits into from
Apr 9, 2024

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.

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 :)

Lib/test/test_doctest/sample_doctest_skip.py Outdated Show resolved Hide resolved
@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.

Lib/test/test_doctest/test_doctest.py Outdated Show resolved Hide resolved
Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
@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
31 checks passed
@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.

diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
…t subprocesses (python#116758)

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
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