gh-109413: Fix some trivial mypy nitpicks in libregrtest #109454
Conversation
AlexWaygood
left a comment
AlexWaygood
left a comment
There was a problem hiding this comment.
Some explanatory comments:
Lib/test/libregrtest/logger.py
Outdated
| formatted_test_time = "%d:%02d:%02d" % (hours, mins, secs) | ||
|
|
||
| line = f"{test_time} {line}" | ||
| line = f"{formatted_test_time} {line}" |
There was a problem hiding this comment.
This variable has been renamed here because there's already a variable earlier in this function called test_time, and it has type float. Mypy enforces that a variable should only ever have one consistent type within any given scope, and here the name is reassigned to a str. We can fix the mypy complaints by using a new variable name for the str value, rather than reusing the test_time name
There was a problem hiding this comment.
test_time is a bad name, in fact, it's just time relative to regrtest start time.
I suggest renaming test_time to log_time and the second variable to formatted_log_time.
There was a problem hiding this comment.
Good shout, I'll do the renames
There was a problem hiding this comment.
In the past, I put "test" everywhere :-) "test" now means nothing, since everything is called "test" in libregrtest :-) That's why I renamed "runtest.py" to "single.py" and rename "runtest_mp.py" to "run_worker.py" ;-)
| report.append(f'skipped={stats.skipped:,}') | ||
| report = ' '.join(report) | ||
| print(f"Total tests: {report}") | ||
| print(f"Total tests: {' '.join(report)}") |
There was a problem hiding this comment.
There's the same issue here: report is a list earlier on in the function, but at this location, it's being assigned to a str. We could use a new variable name, or just do what I suggest here (make it an "anonymous variable")
| except support.ResourceDenied as msg: | ||
| except support.ResourceDenied as exc: | ||
| if not quiet and not pgo: | ||
| print(f"{test_name} skipped -- {msg}", flush=True) | ||
| print(f"{test_name} skipped -- {exc}", flush=True) | ||
| result.state = State.RESOURCE_DENIED | ||
| return | ||
| except unittest.SkipTest as msg: | ||
| except unittest.SkipTest as exc: | ||
| if not quiet and not pgo: | ||
| print(f"{test_name} skipped -- {msg}", flush=True) | ||
| print(f"{test_name} skipped -- {exc}", flush=True) |
There was a problem hiding this comment.
This passage was causing mypy to get very agitated:
libregrtest\single.py:150: error: Assignment to variable "msg" outside
except: block [misc]
msg = f"test {test_name} failed"
^~~~~~~~~~~~~~~~~~~~~~~~~~
libregrtest\single.py:152: error: Trying to read deleted variable "msg"
[misc]
msg = f"{msg} -- {exc}"
^~~~~~
libregrtest\single.py:152: error: Assignment to variable "msg" outside
except: block [misc]
msg = f"{msg} -- {exc}"
^~~~~~~~~~~~~~~~~
libregrtest\single.py:153: error: Trying to read deleted variable "msg"
[misc]
print(msg, file=sys.stderr, flush=True)
^~~
libregrtest\single.py:160: error: Assignment to variable "msg" outside
except: block [misc]
msg = f"test {test_name} failed"
^~~~~~~~~~~~~~~~~~~~~~~~~~
libregrtest\single.py:162: error: Trying to read deleted variable "msg"
[misc]
msg = f"{msg} -- {exc}"
^~~~~~
libregrtest\single.py:162: error: Assignment to variable "msg" outside
except: block [misc]
msg = f"{msg} -- {exc}"
^~~~~~~~~~~~~~~~~
libregrtest\single.py:163: error: Trying to read deleted variable "msg"
[misc]
print(msg, file=sys.stderr, flush=True)
^~~
libregrtest\single.py:176: error: Assignment to variable "msg" outside
except: block [misc]
msg = traceback.format_exc()
^~~~~~~~~~~~~~~~~~~~~~
libregrtest\single.py:177: error: Trying to read deleted variable "msg"
[misc]
print(f"test {test_name} crashed -- {msg}",
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Mypy was getting confused because the msg variable had type Exception here, but lower down in the same function (on line 150), the same variable name is assigned to a str. By renaming the variable here as exc, the exc variable is now always an Exception instance in this function, and the msg variable is now always a str in this function.
| executable = python_cmd | ||
| else: | ||
| executable = [sys.executable] | ||
| executable = (sys.executable,) |
There was a problem hiding this comment.
Mypy complained about this line because on line 26, the executable variable has type tuple[str, ...], but here, the same name is being assigned to an object of type list[str]. We can make mypy happy by just making it a tuple in both the branches.
Lib/test/libregrtest/logger.py
Outdated
| formatted_test_time = "%d:%02d:%02d" % (hours, mins, secs) | ||
|
|
||
| line = f"{test_time} {line}" | ||
| line = f"{formatted_test_time} {line}" |
There was a problem hiding this comment.
test_time is a bad name, in fact, it's just time relative to regrtest start time.
I suggest renaming test_time to log_time and the second variable to formatted_log_time.
3 participants
This PR fixes various mypy nitpicks with
Lib/test/libregrtestthat require small changes to the existing code, but can all be resolved fairly trivially.Currently, if you
cdintoLib/testand then runmypy --config-file regrtest/mypy.ini, mypy reports 23 errors. This PR brings the number of errors reported down to 9.