Add manual local Thread commission test #48
Conversation
MonicaisHer
changed the title
|
When manually setting up these steps, the device chip-all-cluster-minial-app could be successfully commissioned. However, when running the same steps in the above automated go test, the device can't be seen during the commissioning process. At the same time, on the device side, it keeps showing error: Here is the full log. Update: When the chip-tool is installed on the same machine as the OTBR Thread Network, the commissioning process will succeed. Currently, the setup is such that the chip-tool is not installed on the same machine as the OTBR Thread network. |
|
Error from native built chip-all-cluster-minimal-app during the commissioning process: Update: The commission process was successful, and the above DBus error disappeared after rebooting both machines and performing a clean reinstallation of all components. The above error might have been due to an unknown setup environment issue. |
tests/remote_connection.go
Outdated
|
|
||
| // Connect to the remote device | ||
| client := connectToRemoteDevice(t) | ||
| defer client.Close() |
There was a problem hiding this comment.
Client will not close on error because deferred statements don't get called on fatal errors that exit the application and don't return to the caller. Need to use t.Cleanup.
There was a problem hiding this comment.
Make sense. All defer statements have to be moved to t.Cleanup now.
tests/remote_connection.go
Outdated
| session, err := client.NewSession() | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to create session: %v", err) | ||
| } | ||
| defer session.Close() |
There was a problem hiding this comment.
Opening and closing an SSH session for every command isn't ideal. It should be possible to manage all the remote operations for this test within a single SSH session.
There was a problem hiding this comment.
The code has been updated to now open an SSH session only once.
tests/remote_connection.go
Outdated
| // Set remote configuration from environment variables | ||
| setRemoteConfigFromEnv() | ||
|
|
||
| // Connect to the remote device | ||
| client := connectToRemoteDevice(t) |
There was a problem hiding this comment.
The first function reads environment variables and stores those values in global variables. The second function reads those values from global variables. Another global variable is read by installOTBR function.
This coding pattern of loading into global variables and then reading them once elsewhere isn't recommended. True, it makes it easy to write code, but it makes the code equally less maintainable, error-prone, and harder to follow. What if another function modifies the values, or reads them before values are set?
tests/remote_connection.go
Outdated
| } | ||
|
|
||
| // installOTBR installs OTBR and configures Thread network | ||
| func installOTBR(client *ssh.Client, t *testing.T) error { |
There was a problem hiding this comment.
The function name doesn't reflect the functionality. The installation is done remotely.
There was a problem hiding this comment.
Now the OTBR installation has been moved to:
func RemoteDeviceSetup(t *testing.T) {
deployOTBRAgentOnRemoteDevice(t)
}
tests/remote_connection.go
Outdated
| commands := []string{ | ||
| "sudo snap remove --purge openthread-border-router", | ||
| "sudo snap install openthread-border-router --edge", | ||
| "sudo snap set openthread-border-router infra-if=" + infraInterfaceValue, |
There was a problem hiding this comment.
| "sudo snap set openthread-border-router infra-if=" + infraInterfaceValue, | |
| "sudo snap set openthread-border-router infra-if=" + remoteInfraInterfaceEnv, |
There was a problem hiding this comment.
As the current test suite design of the components in each device changes from initial design:
Local Machine:
- chip-tool snap
- OTBR snap with otbr-agent
- all-clusters snap
Remote Machine:
- OTBR snap with Thread network
to
Local Machine:
- chip-tool snap
-OTBR snap with Thread network
Remote Machine:
- OTBR snap with otbr-agent
- all-clusters snap
, this has been updated to remoteInfraInterface.
tests/remote_connection.go
Outdated
| "sudo snap connect openthread-border-router:bluetooth-control", | ||
| "sudo snap connect openthread-border-router:bluez", | ||
| "sudo snap start openthread-border-router", | ||
| "sleep 10", |
There was a problem hiding this comment.
The waiting doesn't require a command executed remotely.
There was a problem hiding this comment.
The wait here has been replaced by:
utils.WaitForLogMessage(t, otbrSnap, "Thread Network", start)
as the Thread network formation changed from the remote device to the local device.
tests/remote_connection.go
Outdated
| } | ||
|
|
||
| // getActiveDataset retrieves the active dataset of Thread network from the remote device | ||
| func getActiveDataset(client *ssh.Client, t *testing.T) (string, error) { |
There was a problem hiding this comment.
| func getActiveDataset(client *ssh.Client, t *testing.T) (string, error) { | |
| func getActiveDataset(t *testing.T, client *ssh.Client) (string, error) { |
For consistency with other function signatures, such as those from testify.
There was a problem hiding this comment.
As a reminder, this was an early draft, but thank you for providing detailed feedback anyway. It has been updated now.
tests/remote_connection.go
Outdated
|
|
||
| // Install OTBR and form Thread network | ||
| if err := installOTBR(client, t); err != nil { | ||
| t.Fatalf("failed to setup remote Thread network: %s", err) |
There was a problem hiding this comment.
Printed error messages should be written as a sentence and start with a capital letter. Same below.
tests/thread_commission_test.go
Outdated
| // Remote device: Access remote device, setup Thread network, and get active dataset | ||
| activeDataset := setupRemoteThreadNetwork(t) | ||
| trimmedActiveDataset := strings.TrimSpace(activeDataset) |
There was a problem hiding this comment.
The active dataset needed to commission should be from a network formed on the local device. The OTBR agent on the remote device is just enabling communication between the matter app and RCP.
There was a problem hiding this comment.
Initial design:
Local Machine:
- chip-tool snap
- OTBR snap with otbr-agent
- all-clusters snap
Remote Machine:
- OTBR snap with Thread network
current design:
Local Machine:
- chip-tool snap
- OTBR snap with Thread network
Remote Machine:
- OTBR snap with otbr-agent
- all-clusters snap
tests/remote_connection.go
Outdated
| "sudo openthread-border-router.ot-ctl dataset init new", | ||
| "sudo openthread-border-router.ot-ctl dataset commit active", | ||
| "sudo openthread-border-router.ot-ctl ifconfig up", | ||
| "sudo openthread-border-router.ot-ctl thread start", |
There was a problem hiding this comment.
There aren't required on the remote side.
There was a problem hiding this comment.
They have been removed from remote side now, as the design for this test suite changes, please see #48 (comment).
tests/thread_commission_test.go
Outdated
| localInfraInterfaceEnv = "LOCAL_INFRA_IF" | ||
| ) | ||
|
|
||
| func TestAllClustersAppThread(t *testing.T) { |
There was a problem hiding this comment.
This test should not be run on GH actions, as it always fails due to the missing remote machine and RCPs dependencies.
There was a problem hiding this comment.
The test has been moved to a standalone directory "manual_thread_tests".
|
Bluetooth related issue: |
|
Thanks for the tests. I could run this after several modifications. Going to merge this into a feature branch to work on fixes, including:
|
* Add manual local Thread commission test (#48) * Remove obsolete LFS file, update file names - Commission via code, without using BLE - Use UTC time on target to ensure consistent queries regardless of local/remote timezones Inline with doc updates: canonical/matter-docs#22 --------- Co-authored-by: Mengyi Wang <[email protected]>
Setup
Local Machine:
Remote Machine:
Testing