GeezerVison

Please don't do that. I want to minimize imports at startup, to minimize how many modules are imported when a test is run. Maybe even add a comment to explain why the import is not done at top level, that it's a deliberate choice.

Okay. The reason why I did this was to accurately add type hints for the TestResults.testsuite_xml attribute on line 35. The current type hints for that attribute are incorrect, and caused several mypy errors.

A possible workaround is to do the top-level import inside an if TYPE_CHECKING block.

A possible workaround is to do the top-level import inside an if TYPE_CHECKING block.

I made that change in 581ef0d; let me know what you think!

If possible, I would prefer avoiding to have to add glue on top of code just to please mypy. If print_warning() API is too weird for mypy, maybe rewrite it with such class directly in test.support, no?

If print_warning() API is too weird for mypy, maybe rewrite it with such class directly in test.support, no?

Unfortunately not. Mypy doesn't know that the test.support module exists: when it comes to the stdlib, it only knows about the modules we include stubs for in typeshed. Historically, we haven't included stubs for any modules in the Lib/test/ directory, as these aren't available on all builds of Python, and are generally only meant to be used by CPython core devs like us. As a result, everything that is imported from test.support is inferred by mypy as being of type Any.

However, that also means that we don't need this protocol here: I only added it to give mypy a little more information, and make the code slightly more type safe. Since you don't like it, I'll remove it!

Mypy doesn't know that the test.support module exists

Can't we make mypy aware of test.support, instead of having to put ducktape to workaround these limitations?

Can't we make mypy aware of test.support

This is somewhat tricky due to the fact that regrtest is in the Lib/test/ directory. Mypy hates being run on stuff inside the Lib/ directory, as it thinks that everything inside the Lib/ directory is "shadowing the stdlib". The only way we're managing to get mypy happy with being run on libregrtest is by cd-ing into Lib/test/, and then running mypy with very specific options so that it doesn't look anywhere else in the Lib/ directory, it only looks in Lib/test/libregrtest.

There would be some ways of making mypy aware of test.support, but I'm not sure I like any of them:

1. We could write a custom script that copies libregrtest and test.support into a temporary directory, then runs mypy on them together. Why I don't like this: it's easier for users if they can run mypy just by invoking mypy on the command line. Having to run mypy via a wrapper script is a bad user experience, and it's more code for us to maintain as well.
2. We could add stubs for test.support in typeshed, so that mypy sees test.support as "just another part of the stdlib". Why I don't like this: lots of Python users can't access test.support on their installed versions of Python, but if we add stubs for test.support in typeshed, mypy will never complain if users import test.support in their own code. Also, it will be a lot of work for us typeshed maintainers to maintain stubs for test.support, since the test.support module tends to change its API a lot more frequently than other things in the Lib/ directory.
3. We could add stubs for test.support inside the libregrtest directory itself. Why I don't like this: this just feels like it would be really ugly, and really hard to maintain. The stubs would go out of date really quickly, if we weren't careful.

We could write a custom script that copies libregrtest and test.support into a temporary directory

That sounds like a good idea to me.

I would prefer that mypy users need workaround, than libregrtest have to write complicated workarounds in the working code just to please mypy.

I don't understand this cast. timeout is a float is use_timeout is True. Why is a cast needed?

Yes, this code is type-safe. The issue is that mypy only understands a limited set of type narrowing operations, and the way timeout has been narrowed to be a float here isn't one of them.

Mypy understands this kind of logic:

x: float | None
if x is not None:
    # OK, mypy understands `f` has to be a float here

But this kind of thing is too complex for mypy:

x: float | None
x_is_None = x is None
if not x_is_None:
    # mypy will not understand that x has been safely narrowed to `None` here

Another way we could make mypy happy would be to add an assert, like this -- but it feels redundant to me, since the type of timeout has already been narrowed to be a float in the lines immediately preceding this:

I dislike assert, it's removed by python -O. Why is this assertion needed? The attribute is declared as json_file: JsonFile | None. Is it to remove the None case?

If mypy doesn't understand the logic, maybe a developer can have the same doubt. Maybe add a regular if json_file is None: raise ....

Or maybe, RunTests can have a sub-type which adds the json_file attribute: the class used by create_worker_process() / worker_process(). Would it please mypy?

Why is this assertion needed? The attribute is declared as json_file: JsonFile | None. Is it to remove the None case?

Yes, it's to remove the None case.

If mypy doesn't understand the logic, maybe a developer can have the same doubt. Maybe add a regular if json_file is None: raise ....

Sure. Should I do the same for all asserts being proposed in this PR?

Or maybe, RunTests can have a sub-type which adds the json_file attribute: the class used by create_worker_process() / worker_process(). Would it please mypy?

Yes, that might also make mypy happier. Hard to tell without seeing a PR and running mypy on it, though :-)