Skip to content
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

Merged

Conversation

petejohanson
Copy link
Contributor

@petejohanson petejohanson commented Jul 17, 2023

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.

  • Like our behavior tests, we build for a posix target, but this one using the BabbleSim board that uses BabbleSim to provide a simulated 2.4GHz PHY.
  • We have a simple posix/BSIM BLE/HoG client, with a few command line flags that can alter it's behavior a bit (e.g. -halt_after_bonding or -wait_on_start=1300) so we can simulate centrals/hosts connecting to our ZMK keyboard and performing disconnects, bonding, etc.
  • Like behavior tests, our ZMK posix peripheral executable uses mock KSCAN to trigger behaviors, e.g. press/release of keycodes, or switching/clearing profiles.
  • The output of the central(s) is captured using pattern matching and then the filtered output is used as a snapshot to compare with the known good snapshot. That allows us to verify what clients see when interacting with ZMK, and verify that doesn't regress/change when we refactor the ZMK side BLE code.
  • Add a new GHA job to run these tests.

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?

@petejohanson petejohanson added enhancement New feature or request core Core functionality/behavior of ZMK bluetooth Bluetooth related items tests labels Jul 17, 2023
@petejohanson petejohanson requested a review from a team as a code owner July 17, 2023 18:33
@petejohanson petejohanson self-assigned this Jul 17, 2023
Copy link
Member

@Nicell Nicell left a 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.

app/tests/ble/central/prj.conf Outdated Show resolved Hide resolved
app/run-ble-test.sh Outdated Show resolved Hide resolved
app/tests/ble/central/src/main.c Show resolved Hide resolved
app/west.yml Show resolved Hide resolved
@petejohanson petejohanson force-pushed the testing/add-bsim-ble-tests branch from 8f30094 to ebfb664 Compare July 18, 2023 07:08
@petejohanson petejohanson requested a review from Nicell July 19, 2023 05:21
@petejohanson petejohanson force-pushed the testing/add-bsim-ble-tests branch from ebfb664 to 98c81cc Compare August 27, 2023 05:01
@petejohanson
Copy link
Contributor Author

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.

@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?

@petejohanson petejohanson force-pushed the testing/add-bsim-ble-tests branch from 98c81cc to dbdf8e1 Compare August 29, 2023 01:04
@petejohanson petejohanson force-pushed the testing/add-bsim-ble-tests branch 2 times, most recently from 0069f43 to 17b1991 Compare October 14, 2023 17:06
@petejohanson petejohanson force-pushed the testing/add-bsim-ble-tests branch from 17b1991 to a56fa58 Compare December 1, 2023 18:59
id: test-dirs
run: |
cd app/tests/ble
export TESTS=$(ls -d * | grep -v central | jq -R -s -c 'split("\n")[:-1]')
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

.github/workflows/test.yml Show resolved Hide resolved
app/run-ble-test.sh Show resolved Hide resolved
app/tests/ble/central/src/main.c Outdated Show resolved Hide resolved
app/tests/ble/central/src/main.c Outdated Show resolved Hide resolved
app/tests/ble/central/src/main.c Outdated Show resolved Hide resolved
app/tests/ble/central/src/main.c Show resolved Hide resolved
app/tests/ble/central/src/main.c Show resolved Hide resolved
@petejohanson petejohanson force-pushed the testing/add-bsim-ble-tests branch 2 times, most recently from 6ad7229 to 482b46f Compare December 2, 2023 06:13
Copy link
Contributor

@caksoylar caksoylar left a 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.

app/run-ble-test.sh Outdated Show resolved Hide resolved
app/run-ble-test.sh Show resolved Hide resolved
app/run-ble-test.sh Outdated Show resolved Hide resolved
app/run-ble-test.sh Outdated Show resolved Hide resolved
app/run-ble-test.sh Outdated Show resolved Hide resolved
app/run-ble-test.sh Outdated Show resolved Hide resolved
app/run-ble-test.sh Outdated Show resolved Hide resolved
app/run-ble-test.sh Outdated Show resolved Hide resolved
@petejohanson petejohanson force-pushed the testing/add-bsim-ble-tests branch 2 times, most recently from 5ebc225 to ed295be Compare December 2, 2023 16:17
@petejohanson petejohanson force-pushed the testing/add-bsim-ble-tests branch from 3160c9b to 7c430d8 Compare December 2, 2023 17:25
Copy link
Contributor

@caksoylar caksoylar left a 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?

@petejohanson
Copy link
Contributor Author

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.

@petejohanson petejohanson merged commit 55aed8e into zmkfirmware:main Dec 3, 2023
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bluetooth Bluetooth related items core Core functionality/behavior of ZMK enhancement New feature or request tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants