8000 gh-109413: Fix some trivial mypy nitpicks in libregrtest by AlexWaygood · Pull Request #109454 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-109413: Fix some trivial mypy nitpicks in libregrtest #109454

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 15, 2023
Merged

gh-109413: Fix some trivial mypy nitpicks in libregrtest #109454

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
95f2e95
gh-109413: Fix some trivial mypy nitpicks in libregrtest
AlexWaygood Sep 15, 2023
24ec358
`test_time` -> `log_time`
AlexWaygood Sep 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
gh-109413: Fix some trivial mypy nitpicks in libregrtest
  • Loading branch information
@AlexWaygood
AlexWaygood committed Sep 15, 2023
commit 95f2e95bc6525b33a5d7458bd3bb6234aa1a4068
4 changes: 2 additions & 2 deletions Lib/test/libregrtest/logger.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ def log(self, line: str = '') -> None:

mins, secs = divmod(int(test_time), 60)
hours, mins = divmod(mins, 60)
test_time = "%d:%02d:%02d" % (hours, mins, secs)
formatted_test_time = "%d:%02d:%02d" % (hours, mins, secs)

line = f"{test_time} {line}"
line = f"{formatted_test_time} {line}"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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 shout, I'll do the renames

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I 8000 n 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" ;-)

if empty:
line = line[:-1]

Expand Down
6 changes: 2 additions & 4 deletions Lib/test/libregrtest/results.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,8 +231,7 @@ def display_summary(self, first_runtests: RunTests, filtered: bool):
report.append(f'failures={stats.failures:,}')
if stats.skipped:
report.append(f'skipped={stats.skipped:,}')
report = ' '.join(report)
print(f"Total tests: {report}")
print(f"Total tests: {' '.join(report)}")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


# Total test files
all_tests = [self.good, self.bad, self.rerun,
Expand All @@ -256,5 +255,4 @@ def display_summary(self, first_runtests: RunTests, filtered: bool):
):
if tests:
report.append(f'{name}={len(tests)}')
report = ' '.join(report)
print(f"Total test files: {report}")
print(f"Total test files: {' '.join(report)}")
8 changes: 4 additions & 4 deletions Lib/test/libregrtest/single.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,14 +136,14 @@ def _runtest_env_changed_exc(result: TestResult, runtests: RunTests,
with saved_test_environment(test_name,
runtests.verbose, quiet, pgo=pgo):
_load_run_test(result, runtests)
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)
Comment on lines -139 to +146
Copy link
Member Author
@AlexWaygood AlexWaygood Sep 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

result.state = State.SKIPPED
return
except support.TestFailedWithDetails as exc:
Expand Down
2 changes: 1 addition & 1 deletion Lib/test/libregrtest/worker.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ def create_worker_process(runtests: RunTests, output_fd: int,
if python_cmd is not None:
executable = python_cmd
else:
executable = [sys.executable]
executable = (sys.executable,)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

cmd = [*executable, *support.args_from_interpreter_flags(),
'-u', # Unbuffered stdout and stderr
'-m', 'test.libregrtest.worker',
Expand Down
0