-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add OTBR snap test #40
Conversation
The snap test failed on GitHub Action because the error, chain 'OTBR_FORWARD_INGRESS' does not exist, preventing OTBR snap from starting. Here are the logs from Github Action testing:
This error doesn't exist when running the test locally. Here are the logs and iptables version:
It might be possible that the iptables and the GitHub Actions container environments are incompatible. If this is the case, a potential solution for this could be to use the 'iptables-nft' package instead of 'iptables'. References: |
The test isn't failing because of that. The given message isn't even an error. It is the output of The logs, from the artifact of the linked test, show the failure clearly:
The error is |
6706852
to
1af74a0
Compare
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 is a good start with decent coverage. See inline comments.
Also, snap options aren't being tested explicitly, i.e. testing of the configure hook.
- Update workflow name - Update module name
Improve naming conventions
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.
Good refactoring and fixes. The testing coverage is reasonable. Thanks.
I've added some comments, which can mostly be taken care of in future work. In that case, please add issues to document them.
tests/snap_services_test.go
Outdated
utils.SnapStart(nil, otbrSnap) | ||
|
||
// Oneshot service | ||
require.False(t, utils.SnapServicesActive(t, otbrSetupApp)) | ||
|
||
// Actice services | ||
require.True(t, utils.SnapServicesActive(t, otbrWebApp)) | ||
require.True(t, utils.SnapServicesActive(t, otbrAgentApp)) | ||
} |
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 starts the services and checks their status. The snap start
operation returns once the oneshot setup service exits. These are all operations that are taken care of by snapd. So in a sense, only the successful completion of the oneshot service is being tested here.
What could make this test more useful is to check if the services complete their startup in a reasonable amount of time (by looking for some reference in the logs), or if they fail.
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.
Issue created: #42
tests/config_test.go
Outdated
command := "sudo snap start openthread-border-router" | ||
_, _ = exec.Command("/bin/bash", "-c", command).CombinedOutput() | ||
t.Logf("[exec] %s", command) | ||
utils.WaitForLogMessage(t, otbrService, expectedLog, start) |
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 not use utils.SnapStart
?
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.
As explained above
|
||
utils.SnapSet(t, otbrSnap, configKey, configValue) | ||
command := "sudo snap start openthread-border-router" | ||
_, _ = exec.Command("/bin/bash", "-c", command).CombinedOutput() |
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.
No error checking.
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 checkSnapOption()
function sometimes sets an invalid config value, such as invalid values for infra-if
and radio-url
. Therefore, in this case, the test does not handle the error as an error is expected. This is also the reason why it doesn't use Utils.Snapstart()
, as the error would be fatal. In this subtest, the crucial aspect is whether the config value has been accepted by the application, rather than whether it is valid.
utils.WaitForLogMessage(t, otbrService, expectedLog, start) | ||
} |
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 matches the prinout from the scripts because xtrace is enabled. Instead, it should be checking the application logs to verify if the configuration has been effective.
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.
Issue created: #43
This PR adds tests and enables GitHub action workflow for OTBR snap.
Testing locally
Run the following command: