-
-
Notifications
You must be signed in to change notification settings - Fork 6
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.
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
base: develop
Are you sure you want to change the base?
Conversation
Signed-off-by: Mateusz Maciejewski <[email protected]>
... 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 |
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 need such a change?
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.
Because we don't want RTE IP check for RTE-less desktop setup.
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.
@matmacieje So that would be a feature request for something like: #198 ?
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.
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
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} |
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.
This assumes we just boot to the target OS as a default one?
What is the problem with this kwd?
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.
This is for Fedora testing on RTE-less setup, with no way to select boot entry.
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.
@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.
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.
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?
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.
@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?
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 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
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.
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.
... sonoff_ip=192.168.10.229 | ||
|
||
# NovaCustom NUC BOX desktop | ||
&{RTE74}= ip=192.168.10.254 |
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.
Is it the one on your desk, or in the lab?
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.
None, this is dummy entry.
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.
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?
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.
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
|
||
|
||
*** Variables *** | ||
${INITIAL_DUT_CONNECTION_METHOD}= SSH |
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.
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
No description provided.