-
Notifications
You must be signed in to change notification settings - Fork 122
linux: add definition for running labgrid tests in lava #611
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.
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
Conversation
|
It needs the changes below in lava to fully work. |
| for key in list(drivers.keys()): | ||
| if key in drivers_to_remove: | ||
| drivers.pop(key) | ||
| conf["drivers"] = {**external_drivers, **conf["drivers"]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conf["drivers"] = {**external_drivers, **drivers}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
|
|
||
| lg_env_file = Path(sys.argv[1]) | ||
| if not lg_env_file.exists(): | ||
| print(f"ERROR: {lg_env_file} not exists") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does not exist*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
automated/linux/labgrid/labgrid.yaml
Outdated
| steps: | ||
| - cd ./automated/linux/labgrid | ||
| # Install | ||
| - apt-get update -y && apt install -y telnet "${DEB_PKGS}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just use apt, not apt-get (and not both).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, also do we always have to install? is there a usecase where we already have everything installed into the rootfs? then maybe we should use the SKIP_INSTALL variable too?
In a way you indicate we always want os python3:3.13...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I CANNOT beleive I used both 😅 .
Switched to apt and added SKIP_INSTALL. It does makse sense to skip the runtime installation and let user use custom docker image.
| # Update env configure | ||
| - if [ "${UPDATE_ENV}" = "true" ] || [ "${UPDATE_ENV}" = "True" ]; then ./update_env.py "${LG_ENV}"; fi | ||
| # Run and create junit style result file | ||
| - pytest -vv --color=no --lg-env "${LG_ENV}" "${LG_TEST}" --junit-xml result.xml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to install pytest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need to specify it here since labgrid requires pytest>=7.0.0.
|
|
||
| # ms -> s | ||
| if txdelay := os.getenv("LAVA_CONNECTION_DELAY"): | ||
| os.environ["LAVA_CONNECTION_DELAY"] = str(int(txdelay) / 1000) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we add a try/except or validate the input? it will raise a ValueError if non-numeric.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added.
automated/linux/labgrid/labgrid.yaml
Outdated
| steps: | ||
| - cd ./automated/linux/labgrid | ||
| # Install | ||
| - apt-get update -y && apt install -y telnet "${DEB_PKGS}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, also do we always have to install? is there a usecase where we already have everything installed into the rootfs? then maybe we should use the SKIP_INSTALL variable too?
In a way you indicate we always want os python3:3.13...
automated/linux/labgrid/labgrid.yaml
Outdated
| - cd ./automated/linux/labgrid | ||
| # Install | ||
| - apt-get update -y && apt install -y telnet "${DEB_PKGS}" | ||
| - pip3 install labgrid PyYAML junitparser==4.0.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check if pip3 exists? It's not guaranteed that the test runs in a container with pip installed. os in metadata is not enforced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a check. python3-pip will be installed if pip3 not found. We need to failover to pip3 install --break-system-packages on recent debian.
Per the desc and README, it suppose to run in a container. I also added SKIP_INSTALL so user can build the depends into their docker image and skip the install at runtime.
| from junitparser import JUnitXml | ||
|
|
||
|
|
||
| def parse_junit(file: str) -> list[str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this should go to automated/lib/py_utill_lib.py ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a script in the run.steps instead of a function. Maybe, move the script to automated/lib? just like the one for parsing rt tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea was that there may be more cases when parsing junit is required. Maybe moving the whole script is a good idea. Let's try this and refactor in the future if required.
|
@bhcopeland @roxell @mwasilew Thank you for the comments. Hopefully, I have addressed all of them. Let me know if there are more :) |
automated/linux/labgrid/labgrid.yaml
Outdated
| if [ "${SKIP_INSTALL}" = "false" ] || [ "${SKIP_INSTALL}" = "False" ]; then | ||
| which pip3 || DEB_PKGS="${DEB_PKGS} python3-pip" | ||
| apt update -y && apt install -y telnet ${DEB_PKGS} | ||
| PYPI_PKGS="labgrid PyYAML junitparser==4.0.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this up to the 'params' section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we have to pin junitparser?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switched to >=4.0.2.
| @@ -0,0 +1,49 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add copyright
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added.
Signed-off-by: Chase Qi <chase.qi@linaro.org>
No description provided.