Added type annotations and removed lints for run_tests.py #180597
Conversation
There was a problem hiding this comment.
Code Review
This pull request focuses on improving the run_tests.py script by adding type annotations and removing lints. The changes significantly enhance code clarity and maintainability by introducing type hints to function signatures and replacing old-style string formatting with f-strings. Additionally, file handling is improved by using context managers and specifying encoding. I've identified a couple of areas for further improvement: one related to a redundant call and another to improve exception handling.
|
|
||
| try: | ||
| run_cmd([adb_path, 'shell', remote_tests_path]) | ||
| except: |
There was a problem hiding this comment.
Using a bare except: is generally discouraged as it catches all exceptions, including system-exiting ones like SystemExit or KeyboardInterrupt, making it harder to interrupt the program. It's better to catch a more specific exception. Since the intention here is to catch any error from run_cmd and then re-raise it, except Exception: would be more appropriate.
| except: | |
| except Exception: |
References
- The Google Python Style Guide advises against using bare
except:clauses. It recommends catching more specific exceptions orException. (link)
| logger.info(line.rstrip()) | ||
|
|
||
| process.wait() | ||
| process.wait() |
There was a problem hiding this comment.
The with subprocess.Popen(...) as process: statement ensures that process.wait() is called when the with block is exited. This explicit call to process.wait() is redundant and can be removed.
There was a problem hiding this comment.
This suggestion makes sense to me.
There was a problem hiding this comment.
redundant now that we can't use popen with with statements
b-luk
left a comment
There was a problem hiding this comment.
LGTM, with the caveat that I haven't written any Python code for many years (and never Python3 or Python with type annotations). So if you want someone with actual Python experience to review it, make sure to include another reviewer.
| message.append(f'ninja -C {ios_out_dir} ios_test_flutter') | ||
| final_message = ( | ||
| f"{ios_out_dir} or {ios_test_lib} doesn't exist. " | ||
| f'Please run the following commands: \n{chr(10).join(message)}' |
There was a problem hiding this comment.
Is chr(10).join() preferred over "\n".join()?
There was a problem hiding this comment.
Maybe just get rid of this whole message-list/final_message/'\n'-join thing and have one single message f-string. I'm not seeing why this was made to be this slightly convoluted message/final_message/join thing thing.
There was a problem hiding this comment.
lol no, gemini did it automatically and I missed it. thanks. i'll fix it
There was a problem hiding this comment.
Oh, it looks like f-strings doesn't handle having string literals in the {} expressions.
There was a problem hiding this comment.
Oh, it looks like f-strings doesn't handle having string literals in the {} expressions.
That's surprising. We do have {" ".join(flags)}'] and {",".join(all_types)} in f-strings elsewhere in this file.
There was a problem hiding this comment.
I think it may be some weird thing with the fact there are 2 fstrings back to back? The linter will complain about the previous line actually: testing/run_tests.py:717:7: E0001: f-string expression part cannot include a backslash (<unknown>, line 717) (syntax-error)
Looked at it a bit more, I'm actually not sure why it freaks out here.
| logger.info(line.rstrip()) | ||
|
|
||
| process.wait() | ||
| process.wait() |
There was a problem hiding this comment.
This suggestion makes sense to me.
| # TODO ricardoamador: remove this check when python 2 is deprecated. | ||
| version_output = version_output if isinstance(version_output, | ||
| str) else version_output.decode(ENCODING) | ||
| version_output = version_output.strip() | ||
| match = re.match(r'Xcode (\d+)', version_output) | ||
| version_output_str = version_output if isinstance(version_output, | ||
| str) else version_output.decode(ENCODING) |
There was a problem hiding this comment.
Is Python2 still used with this? I think subprocess.Popen with a with block isn't supported in Python2, so maybe this file is supposed to be Python3-only.
If this is only for Python3, I think the TODO is suggesting to simplify this to always do .decode(ENCODING).
There was a problem hiding this comment.
You're right. It looks like it was introduced in 3.2. We must be using that on ci. We use the 2.7 version of pylint, I would have expected it to flag this, but doesn't look like it can.
There was a problem hiding this comment.
Removed them, based on the fact we are using the 2.7 version of pylint i'll assume we are targeting that.
|
autosubmit label was removed for flutter/flutter/180597, because - The status or check suite Linux linux_web_engine_tests has failed. Please fix the issues identified (or deflake) before re-applying this label. |
This passes mypy and pylint. I looked into introducing mypy into ci, but it didn't make sense considering the small number of python scripts.
Test exempt: this is a test
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.