-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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 BSIM based BLE tests #1875
Add BSIM based BLE tests #1875
Conversation
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.
Basic cleanup and a question. This is really cool! I'll need to look closer at how the main.c
works, but I'm really impressed with the test run on this PR.
I noticed in the logs from the test run that the start, test logs, and pass/fail logging to GitHub actions are all jumbled up between different tests. It'd be nice if they could be grouped for better readability.
8f30094
to
ebfb664
Compare
ebfb664
to
98c81cc
Compare
@Nicell Ok, I did some clean up so the verbose output will only show when running a single test during development, otherwise it's quieter. See https://github.com/zmkfirmware/zmk/actions/runs/5988986557/job/16244737224?pr=1875#step:10:1 for example. Thoughts? |
98c81cc
to
dbdf8e1
Compare
0069f43
to
17b1991
Compare
17b1991
to
a56fa58
Compare
id: test-dirs | ||
run: | | ||
cd app/tests/ble | ||
export TESTS=$(ls -d * | grep -v central | jq -R -s -c 'split("\n")[:-1]') |
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 would be nice to not have the test cases and the test builds be sibling folders. If we ever need to add a "peripheral" build, then this command has to become more complicated.
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.
Also not sure it's worth the complexity trying to generalize it at this point.
app/tests/ble/profiles/bond-clear-then-bond-second-client/snapshot.log
Outdated
Show resolved
Hide resolved
6ad7229
to
482b46f
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.
I noted some fixes for issues that shellcheck brought up, feel free to ignore any that you consider nitpicky.
5ebc225
to
ed295be
Compare
3160c9b
to
7c430d8
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.
LGTM for the parts I reviewed. It doesn't look like it needs any docs updates, does it?
It could use some docs on how to add the tests, but nothing that's end user affecting that I think is a must have for this PR. |
In preparation for some major refactors of our BLE code to use State Machine Framework, this PR adds some core integration tests for our BLE code.
-halt_after_bonding
or-wait_on_start=1300
) so we can simulate centrals/hosts connecting to our ZMK keyboard and performing disconnects, bonding, etc.I've layers my WIP BLE changes for SMF onto this for testing, and it properly caught a regression there, and then I have them still passing with my changes at this point.
This is a baseline set of 5 tests, but would like to supplement with more as we go.
Thoughts?