micropython: support armv7r5f [do not merge]#467
micropython: support armv7r5f [do not merge]#467mateusz-bloch wants to merge 2 commits intomasterfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces foundational support for running MicroPython tests on the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new test harness for MicroPython on the armv7r5f architecture. The changes primarily involve setting up the test environment, interacting with an editor to load test scripts, and executing them. While the overall structure is in place, several areas require attention to improve correctness, robustness, and maintainability. Specifically, the editor interaction logic relies on fragile workarounds and hardcoded control characters, and there's a critical error in how expected outputs are handled for tests requiring regex matching. Additionally, the hardcoded root path for MicroPython tests could be made more flexible.
micropython/micropython_armv7r5f.py
Outdated
| else: | ||
| expected = test_name |
There was a problem hiding this comment.
In the else branch, when test_name is in TESTS_WITH_REGEX_OUTPUT, expected is incorrectly set to test_name. The expected variable should contain the content of the .exp file, processed as a regular expression, not the test file's name. This logic will cause tests that require regex output matching to fail.
| else: | |
| expected = test_name | |
| expected = create_regex(expected_output) |
micropython/micropython_armv7r5f.py
Outdated
| from trunner.types import Status, TestResult | ||
|
|
||
| import psh.tools.psh as psh | ||
|
|
There was a problem hiding this comment.
The create_regex function, which is essential for correctly processing expected output for tests with regex output, is defined in micropython/micropython.py but not imported into this file. It needs to be imported to be used in the harness function.
| from micropython.micropython import create_regex |
micropython/micropython_armv7r5f.py
Outdated
| # Hack | ||
| for _ in range(attempts): | ||
| dut.sendline("") | ||
|
|
There was a problem hiding this comment.
The comment "# Hack" indicates this is a workaround. Sending empty lines repeatedly to wait for the editor to be ready is fragile and can lead to flaky tests if the editor's startup time varies. It's better to expect a specific prompt or a known editor state to ensure readiness, making the test more robust and less prone to timing issues.
micropython/micropython_armv7r5f.py
Outdated
| dut.send("\x13") # Ctrl+S | ||
| dut.send("\x11") # Ctrl+Q |
There was a problem hiding this comment.
Using hardcoded control characters like "\x13" (Ctrl+S) and "\x11" (Ctrl+Q) for editor operations makes this script highly dependent on the specific editor's keybindings, reducing maintainability and portability. If the underlying editor changes or has different keybindings, this part of the test will break. Consider using a more abstract way to interact with the editor if available, or document this dependency clearly.
micropython/micropython_armv7r5f.py
Outdated
| MICROPYTHON_ROOT = ( | ||
| Path(__file__).resolve().parent | ||
| / "../../_fs/armv7r5f-zynqmp-qemu/root/usr/test/micropython" | ||
| ).resolve() |
There was a problem hiding this comment.
The MICROPYTHON_ROOT path is hardcoded, making the test harness less flexible and prone to breakage if the project's file layout changes. Consider making this path configurable, perhaps through a TestContext parameter or an environment variable, to improve portability and maintainability.
Unit Test Results 1 files ± 0 2 027 suites +1 402 6h 3m 55s ⏱️ + 5h 12m 23s For more details on these failures, see this check. Results for commit 43bdb70. ± Comparison against base commit f7978fc. This pull request removes 1942 and adds 14719 tests. Note that renamed tests count towards both.This pull request removes 134 skipped tests and adds 3257 skipped tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
mateusz-bloch
force-pushed
the
mateusz-bloch/micropython_for_armv7r5f
branch
19 times, most recently
from
fa10b35 to
bbabae8
Compare
JIRA: CI-645
mateusz-bloch
force-pushed
the
mateusz-bloch/micropython_for_armv7r5f
branch
from
bbabae8 to
43bdb70
Compare
1 participant
JIRA: CI-645
Description
Motivation and Context
Types of changes
How Has This Been Tested?
Checklist:
Special treatment