Skip to content

Add unit tests for Utils::toHex#925

Merged
liamcottle merged 2 commits intomeshcore-dev:devfrom
mtlynch:unit-tests
May 1, 2026
Merged

Add unit tests for Utils::toHex#925
liamcottle merged 2 commits intomeshcore-dev:devfrom
mtlynch:unit-tests

Conversation

@mtlynch
Copy link
Copy Markdown

@mtlynch mtlynch commented Oct 12, 2025

Adds unit tests for Utils::toHex using Google gtest framework and adds automatic unit tests runs on every commit to Github Actions for the repo.

@mtlynch
Copy link
Copy Markdown
Author

mtlynch commented Oct 12, 2025

I know toHex isn't the most interesting function to test, but this is just a proof of concept of the idea of unit testing in this project.

Unit testing can make it significantly easier and faster to catch bugs, as we can exercise logic on regular desktops and in Github Actions without having to push code to a LoRa device and test it. It also allows maintainers to see immediately in Github Actions if a change caused a regression in tests.

I chose toHex just because it's a simple function with no external dependencies, though because it's defined in a file that also includes dependencies on Stream.h and SHA256.h, we need to mock out those dependencies.

@446564
Copy link
Copy Markdown
Contributor

446564 commented Apr 24, 2026

@mtlynch can you resolve the conflicts on this one?

@446564
Copy link
Copy Markdown
Contributor

446564 commented Apr 24, 2026

testing now :)

@mtlynch
Copy link
Copy Markdown
Author

mtlynch commented Apr 24, 2026

@446564 - Sure, just fixed. The first two pulled in some unintended changes from the dev/main branches, but it's fixed now.

@446564
Copy link
Copy Markdown
Contributor

446564 commented Apr 24, 2026

@mtlynch ok newb time, haven't run unit tests with pio before and getting an error: .pio/libdeps/native/googletest/googletest/include/gtest/internal/gtest-port.h:273:2: error: #error C++ versions less than C++17 are not supported. am I missing some dep or config not present in the repo?

edit: I have gcc 14 which supports c++17 but this should be using the pio installed toolchain right?

With the '^x.y.z' semantics, the version can upgrade out from under us and change dependencies, so this pins to an exact version.
@mtlynch
Copy link
Copy Markdown
Author

mtlynch commented Apr 24, 2026

@446564 - Yeah, it was my fault. I specified gtest ^1.15.2, which was latest when I wrote it six months ago, but now the latest is 1.17.0, which apparently depends on c++17, so I've specified c++ 17 and pinned to the exact version so that future updates don't change out from under us.

Fixed in b6a3aa1

@446564
Copy link
Copy Markdown
Contributor

446564 commented Apr 24, 2026

it's working now, i don't like the output, it's weird to me. but maybe that's just testing with pio

@mtlynch
Copy link
Copy Markdown
Author

mtlynch commented Apr 24, 2026

@446564 - Glad it's working! What's remaining to get this merged?

@liamcottle
Copy link
Copy Markdown
Member

Have pulled it in, seems to work for me. I agree with @446564 that the output isn't very nice, compared to other testing frameworks. But I'm sure as more tests are added we will just get used to it.

@446564
Copy link
Copy Markdown
Contributor

446564 commented Apr 25, 2026

Have pulled it in, seems to work for me. I agree with @446564 that the output isn't very nice, compared to other testing frameworks. But I'm sure as more tests are added we will just get used to it.

Agreed, or even just rework how they are done as we learn.

@mtlynch
Copy link
Copy Markdown
Author

mtlynch commented Apr 25, 2026

Yeah, I was looking for just whatever unit test library is compatible with PlatformIO and requires the least churn. If we find one that works better, it shouldn't be too much work to port to a different unit testing framework.

@mtlynch
Copy link
Copy Markdown
Author

mtlynch commented Apr 29, 2026

@liamcottle - Is there anything I can do to help get this merged?

@446564
Copy link
Copy Markdown
Contributor

446564 commented Apr 29, 2026

I was messing with unity yesterday, which is the default framework for pio and it has nicer output but not sure what direction we want to go or if there is a reason we need gtest.

Otherwise we can merge imo.

@ripplebiz
Copy link
Copy Markdown
Member

OK, we've been discussing unit test strategy, and think that for native that googletest will be the goto.
On-device behaviours testing can wait, for now

@liamcottle
Copy link
Copy Markdown
Member

Great, thanks all.

Scott and I have decided to just merge this for now.

We were looking into reasons why Google Test was chosen over Unity, and possibly Catch2, but from looking around Google Test seems to be one of the more popular frameworks for c++ testing.

The majority of my testing experience is based in PHP, Java and JavaScript projects. Not so much embedded/c++.

At this stage, I think we will just focus on unit testing. Ensuring classes, helper functions and parsers etc are all behaving as expected with provided inputs.

Further down the track, we can probably look at some minimal on-device testing, for things such as the sleep/wake logic, which in some cases could cause some strange behaviour.

Since the unit tests need to run natively in environments such as GitHub Actions, as opposed to the microcontrollers, we're happy to merge this in and see where it takes us. We can just deal with the output format of Google Test. As long as things are passing, and we can see what is not passing, that's all that matters for now.

Also, wanted to apologise for how long it's taken us to get this looked at and merged in. The project has grown significantly in a short while, and we're all being pulled and pushed in different directions.

Lots of PRs coming through and we need to be selective as to what is accepted to avoid the project becoming spaghetti and becoming unstable/unreliable.

Merging :)

@liamcottle liamcottle merged commit ccda0a9 into meshcore-dev:dev May 1, 2026
12 checks passed
@liamcottle
Copy link
Copy Markdown
Member

Just merged in, then saw another email notification about this... #2280 (review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants