Skip to content

Conversation

@mhsmith
Copy link
Member

@mhsmithmhsmith 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-appbedevere-appbot added the tests Tests in the Lib/test dir label Mar 13, 2024
@mhsmith
Copy link
MemberAuthor

@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

@sobolevnsobolevn 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...
ifsupport.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

defdoctest_requires_subproccess(func): ifnotsupport.has_subprocess_support: func.__doc__=None# skip the doctest by setting it to `None`returnfunc@doctest_requires_subprocessdeftest_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
MemberAuthor

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-storchakaserhiy-storchaka self-requested a review March 21, 2024 21:19
Copy link
Member

@serhiy-storchakaserhiy-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
MemberAuthor

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.

@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-storchakaserhiy-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

@sobolevnsobolevn 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
MemberAuthor

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

@sobolevnsobolevn 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.

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
@mhsmith
Copy link
MemberAuthor

@sobolevn: Are you able to merge this PR?

@sobolevn
Copy link
Member

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

@sobolevnsobolevn merged commit 22b25d1 into python:mainApr 9, 2024
@sobolevn
Copy link
Member

What do you think about 3.12 backport?

@mhsmith
Copy link
MemberAuthor

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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip newstestsTests in the Lib/test dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@mhsmith@sobolevn@serhiy-storchaka