cpython icon indicating copy to clipboard operation
cpython copied to clipboard

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

Open mhsmith opened this issue 1 year ago โ€ข 2 comments

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.

  • Issue: gh-116622

mhsmith avatar Mar 13 '24 20:03 mhsmith

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

mhsmith avatar Mar 21 '24 12:03 mhsmith

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

sobolevn avatar Mar 21 '24 19:03 sobolevn

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.

sobolevn avatar Mar 22 '24 05:03 sobolevn

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

serhiy-storchaka avatar Mar 22 '24 18:03 serhiy-storchaka

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 avatar Mar 27 '24 17:03 mhsmith

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.

sobolevn avatar Mar 27 '24 17:03 sobolevn

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. ๐Ÿ˜„

mhsmith avatar Mar 27 '24 19:03 mhsmith

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

I will take it from here, thank you for this PR! ๐Ÿ‘

sobolevn avatar Mar 27 '24 19:03 sobolevn

@sobolevn: Are you able to merge this PR?

mhsmith avatar Apr 04 '24 21:04 mhsmith

Yes, sure! Thanks a lot for working on this ๐Ÿ‘

sobolevn avatar Apr 09 '24 11:04 sobolevn

What do you think about 3.12 backport?

sobolevn avatar Apr 09 '24 11:04 sobolevn

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.

mhsmith avatar Apr 09 '24 14:04 mhsmith