Skip to content

Temporary changes for NUC BOX MTL testing, do not merge. #1013

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Temporary changes for NUC BOX MTL testing, do not merge. #1013

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion keywords.robot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,7 @@ Prepare To SSH Connection
... sections if the communication with the platform based on
... the SSH protocol
# tu leci zmiana, musimy brać platformy zgodnie z tym co zostało pobrane w dasharo
Open Connection And Log In
# Open Connection And Log In
Copy link
Contributor

@macpijan macpijan Aug 19, 2025

Choose a reason for hiding this comment

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

Why do we need such a change?

Copy link
Contributor Author

@matmacieje matmacieje Aug 19, 2025

Choose a reason for hiding this comment

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

Because we don't want RTE IP check for RTE-less desktop setup.

Copy link
Contributor

@macpijan macpijan Aug 19, 2025

Choose a reason for hiding this comment

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

@matmacieje So that would be a feature request for something like: #198 ?

Copy link
Contributor

@philipanda philipanda Aug 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The change is already merged https://github.com/Dasharo/open-source-firmware-validation/pull/1006/files
The line was added somewhat recently and is not needed unless someone wants to use RTE alongside SSH as the main connection method, but that breaks tests without an RTE

VAR ${PLATFORM}= ${CONFIG} scope=GLOBAL
IF '${DEFAULT_BOOT_OS_ID}'
Import Variables ${CURDIR}/os-config/${DEFAULT_BOOT_OS_ID}-credentials.py
Expand Down
2 changes: 1 addition & 1 deletion lib/options/options-lib_uefi-setup-menu.robot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ Measure Coldboot Time
# Do one more iteration than requested, as we may hit first boot which is always longer.
FOR ${index} IN RANGE 0 ${iterations}+1
Power Cycle On
Boot System Or From Connected Disk ${os_id}
# Boot System Or From Connected Disk ${os_id}
Copy link
Contributor

@macpijan macpijan Aug 19, 2025

Choose a reason for hiding this comment

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

This assumes we just boot to the target OS as a default one?

What is the problem with this kwd?

Copy link
Contributor Author

@matmacieje matmacieje Aug 19, 2025

Choose a reason for hiding this comment

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

This is for Fedora testing on RTE-less setup, with no way to select boot entry.

Copy link
Contributor

@macpijan macpijan Aug 19, 2025

Choose a reason for hiding this comment

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

@matmacieje So with SSH? For SSH this kwd should be skipped: https://github.com/Dasharo/open-source-firmware-validation/blob/develop/lib/bios/menus.robot#L1287

So I wonder what's the exact issue here, so we can solve it properly.

Copy link
Contributor

@philipanda philipanda Aug 20, 2025

Choose a reason for hiding this comment

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

Is this change required?
If you check the definition of the keyword, it only does anything if the chosen OS is not already booted.
Maybe there is some bug in this keyword, that made it not work for you?

Copy link
Contributor

@macpijan macpijan Aug 27, 2025

Choose a reason for hiding this comment

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

@matmacieje I see in the 2nd PR you also commented it out. Both me and @philipanda think it's not necessary. Can you please prove us wrong e.g. by attaching some logs?

Copy link
Contributor Author

@matmacieje matmacieje Aug 27, 2025

Choose a reason for hiding this comment

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

I did it to avoid recurring errors of Boot Menu parsing (no Ubuntu entry on the list). Example:
0_9_0_rc4_dcu_2025_08_25_13_18_54.zip

Copy link
Contributor

@philipanda philipanda Aug 27, 2025

Choose a reason for hiding this comment

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

The keywords that parse the boot menu remove a hardcoded amount of lines from the start and from the end of the menu to get rid of unwanted lines that do not contain actual boot entries. If some newline is lost because of unstable serial connection, then this way of parsing the menu fails. In that case it is reasonable to temporarily comment this and allow the device to boot to the default boot option just to allow the tests to run. This change should not be merged though.
Fuzzy string comparisons can't help with newlines being lost when we parse the menus as lists.

A way to fix this would be to search for some characteristic strings in the setup menu instead of hardcoding the amount of lines to drop, but lost newlines separating the boot entries could still break all of this.

I guess the most robust way to overcome this problem would be to re-enter the boot menu if errors are encountered. That would make the execution longer if errors occur, but it won't fail.

Such extreme workaround should be implemented with care, especially that we have reasons to suspect that the errors on serial are caused by the implementation of the USB serial console in EDK2, as there are no serial console errors once the device boots into an OS.

Login To Linux
Switch To Root User
${boot_time}= Get Boot Time From Cbmem
Expand Down
2 changes: 1 addition & 1 deletion lib/terminal.robot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ Set DUT Response Timeout
${prev_timeout}= Telnet.Set Timeout ${timeout}
ELSE IF '${DUT_CONNECTION_METHOD}' in ['SSH', 'open-bmc']
${con}= Get Connection
VAR ${prev_timeout}= ${con.timeout}=
VAR ${prev_timeout}= ${con.timeout}
SSHLibrary.Set Client Configuration timeout=${timeout}
ELSE
FAIL Unknown connection method: ${DUT_CONNECTION_METHOD}
Expand Down
2 changes: 1 addition & 1 deletion os-config/202-credentials.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

DEVICE_OS_USERNAME = "linux"
DEVICE_OS_PASSWORD = "linux"
DEVICE_OS_HOSTNAME = "3mdeb"
DEVICE_OS_HOSTNAME = "fedora"

DEVICE_OS_USER_PROMPT = f"[{DEVICE_OS_USERNAME}@{DEVICE_OS_HOSTNAME} ~]$"
DEVICE_OS_ROOT_PROMPT = f"[root@{DEVICE_OS_HOSTNAME} {DEVICE_OS_USERNAME}]#"
1 change: 1 addition & 0 deletions platform-configs/include/default.robot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ ${SB_ERROR_STRING}= The image signature is inval

# See: https://github.com/Dasharo/dasharo-issues/issues/614
${LAPTOP_EC_SERIAL_WORKAROUND}= ${FALSE}
${LAPTOP_PLATFORM}= ${TBD}

# Library config
# Option library: UEFI configuration variable backend.
Expand Down
2 changes: 1 addition & 1 deletion platform-configs/include/novacustom-nuc_box.robot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ ${WIFI_CARD}= Intel Corporation Meteor Lake PCH CNVi W

${DEFAULT_BOOT_OS_ID}= ${ENV_ID_UBUNTU}
@{TESTED_LINUX_DISTROS}=
... ${ENV_ID_UBUNTU} # ${ENV_ID_FEDORA}
... ${ENV_ID_UBUNTU}
${TESTS_IN_WINDOWS_SUPPORT}= ${TRUE}

${CLEVO_USB_C_HUB}= Billboard Device
Expand Down
11 changes: 11 additions & 0 deletions platform-configs/novacustom-nuc_box-125H.robot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,17 @@ Resource include/default.robot


*** Variables ***
${INITIAL_DUT_CONNECTION_METHOD}= SSH
Copy link
Contributor

@philipanda philipanda Aug 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It might be useful to create some common config that defines the testing mode, like RTE-only, RTE+sonoff, SSH+sonoff, SSH+none.
There is a limited amount of working configs, and many other variables are often fully dependent on the selected combination of DUT_CONNECTION_METHOD and POWER_CTRL. Example: TESTS_IN_FIRMWARE_SUPPORT is only TRUE if DUT_CONNECTION_METHOD is either Telnet, or PiKVM
Most of this added block could be separated from most configs to make it easier to choose from the modes, or even use an argument to robot to switch between them instead of doing such modifications on most regressions

${DUT_CONNECTION_METHOD}= SSH
${POWER_CTRL}= sonoff
# ${OPTIONS_LIB}= options-lib_dcu
${TESTS_IN_FIRMWARE_SUPPORT}= ${FALSE}
${LAPTOP_PLATFORM}= ${FALSE}

${DEFAULT_BOOT_OS_ID}= ${ENV_ID_FEDORA}
@{TESTED_LINUX_DISTROS}=
... ${ENV_ID_FEDORA}

${CPU}= Intel(R) Core(TM) Ultra 5 125H
${DEF_CORES_PER_SOCKET}= 14
${DEF_THREADS_TOTAL}= 18
Expand Down
1 change: 1 addition & 0 deletions platform-configs/novacustom-nuc_box-155H.robot
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ ${DEF_THREADS_TOTAL}= 22
# TODO: remove, the value below can be inferred from the one above
${DEF_ONLINE_CPU}= 0-21
${DEF_SOCKETS}= 1
${LAPTOP_PLATFORM}= ${FALSE}

${CPU_MAX_FREQUENCY}= 4800
${CPU_MIN_FREQUENCY}= 200
Expand Down
7 changes: 6 additions & 1 deletion variables.robot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,11 @@ ${OS_UBUNTU}= ubuntu
... platform_vendor=Clevo
... sonoff_ip=192.168.10.229

# NovaCustom NUC BOX desktop
&{RTE74}= ip=192.168.10.254
Copy link
Contributor

@macpijan macpijan Aug 19, 2025

Choose a reason for hiding this comment

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

Is it the one on your desk, or in the lab?

Copy link
Contributor Author

@matmacieje matmacieje Aug 19, 2025

Choose a reason for hiding this comment

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

None, this is dummy entry.

Copy link
Contributor

@macpijan macpijan Aug 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

A dummy entry to get past through the inability of running tests without RTE? So again #198 or similar.

Is RTE required by OSFV also when we are running tests over SSH?

Copy link
Contributor

@philipanda philipanda Aug 20, 2025

Choose a reason for hiding this comment

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

It was definitely not needed a couple months ago.
I guess modifying variables.robot shouldn't be needed for SSH tests with this line removed https://github.com/Dasharo/open-source-firmware-validation/pull/1013/files#diff-82f3f16c416be69886d34f9df8144e79ff3ebc7fb8f1382f45f69618752fc409R94

... platform=novacustom-nuc_box
... platform_vendor=Clevo

@{RTE_LIST}= &{RTE05} &{RTE06} &{RTE07} &{RTE08} &{RTE09}
... &{RTE10} &{RTE11} &{RTE12} &{RTE13} &{RTE14}
... &{RTE15} &{RTE16} &{RTE17} &{RTE18} &{RTE19}
Expand All @@ -286,7 +291,7 @@ ${OS_UBUNTU}= ubuntu
... &{RTE55} &{RTE56} &{RTE57} &{RTE58} &{RTE59}
... &{RTE60} &{RTE61} &{RTE62} &{RTE63} &{RTE64}
... &{RTE65} &{RTE66} &{RTE67} &{RTE68} &{RTE69}
... &{RTE70} &{RTE71} &{RTE72} &{RTE73}
... &{RTE70} &{RTE71} &{RTE72} &{RTE73} &{RTE74}

# hardware database:
# -----------------------------------------------------------------------------
Expand Down