add some unit tests

[mkinney]

Describe the bug
Would like to see some unit tests added to this repo.

To Reproduce
Steps to reproduce the behavior:

1. Be able to run some sort of quick (less than 10 seconds) test to validate code.

Expected behavior
A way to unit test some of the code.

Screenshots
See https://docs.platformio.org/en/latest/plus/unit-testing.html

Device info:
NA

Smartphone information (if relevant):
NA

Additional context
Add any other context about the problem here.

[mc-hamster]

We have a simulated device and run tests on that. Is this adequate? It's 20 seconds of tests.

[mkinney]

So, that's not really a unit test. But, it is a great thing to know about.

I'll poke around the code to see if I can find a great first candidate for a unit test.

[mc-hamster]

Ok! Jm Casler Always Agile & Life Long Maker https://www.casler.org <http://www.casler.org/> +1 408 806 8098

…

On Mon, Jan 10, 2022 at 4:54 PM mkinney ***@***.***> wrote: So, that's not really a unit test. But, it is a great thing to know about. I'll poke around the code to see if I can find a great first candidate for a unit test. — Reply to this email directly, view it on GitHub <#1045 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADBWG7LN5ICIN4X5IUOY77DUVN5T3ANCNFSM5LACHUYQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you commented.Message ID: ***@***.***>

[mkinney]

Found one. Again, starting small.

https://github.com/meshtastic/Meshtastic-device/blob/master/src/mesh/Channels.cpp#L18

Since code is so small, I'll include it here:

uint8_t xorHash(const uint8_t *p, size_t len)
{
    uint8_t code = 0;
    for (size_t i = 0; i < len; i++)
        code ^= p[i];
    return code;
}

So, we need a simple test harness to exercise that code.

There are several open source unit test frameworks, and maybe pio even has some built in.

[mkinney]

Yeah, pio does. Looking at the docs above.

[mkinney]

I tried using the pio test but the test could not find the code/library.

cat test/test_channel.cpp
#include <Arduino.h>
#include <unity.h>

#include "Channels.h"

void test_one(void) {
    Channels channels;
    channels.initDefaults();
    TEST_ASSERT_EQUAL_STRING(channels.getName(0), "");
}

void process() {
    UNITY_BEGIN();
    RUN_TEST(test_one);
    UNITY_END();
}


int main(int argc, char **argv) {
    process();
    return 0;
}

pio test gives me this:

.pio/build/tbeam/test/test_channel.cpp.o:(.literal._Z8test_onev+0x4): undefined reference to `Channels::initDefaults()'
.pio/build/tbeam/test/test_channel.cpp.o:(.literal._Z8test_onev+0x8): undefined reference to `Channels::getName(unsigned int)'

I also tried using https://github.com/mity/acutest but I ran into an issue I don't quite know how to get around. I started to surround some includes with #IFNDEF UNIT_TEST and got pretty far.

$ cat Makefile
CC = g++
FLAGS = -I${HOME}/.platformio/packages/framework-arduinoespressif32/cores/esp32/ -I../src/mesh/generated/ -DARDUINO=100 -I../src/esp32/ -I{HOME}/.platformio/packages/framework-arduinoespressif32/tools/sdk/include/wpa_supplicant/ -I../lib/nanopb/include -DAPP_VERSION=2 -DAPP_VERSION_SHORT=2 -DHW_VERSION=10 -DUNIT_TEST=true
#FLAGS = -I${HOME}/.platformio/packages/framework-arduinoespressif32/cores/esp32/ -DARDUINO=100 -I../lib/nanopb/include -DAPP_VERSION=2 -DAPP_VERSION_SHORT=2 -DHW_VERSION=10 -DUNIT_TEST=true -fexceptions

test: FORCE
	$(CC) $(FLAGS) -I../src -I../src/mesh ../src/mesh/generated/deviceonly.pb.c ../src/mesh/generated/mesh.pb.c ../src/mesh/generated/channel.pb.c ../src/mesh/Channels.cpp ../src/mesh/CryptoEngine.cpp ../src/esp32/ESP32CryptoEngine.cpp test_mesh_channels.cpp

# Makefile hack to get the examples to always run
FORCE: ;

$ cat test_mesh_channels.cpp
#include "acutest.h"

#include "Channels.h"

void test_mesh_channels_initDefaults(void) {
    Channels channels;
    channels.initDefaults();
}

TEST_LIST = {
	{"test-mesh-channels-initDefaults", test_mesh_channels_initDefaults},
	{NULL, NULL}
};

I cannot seem to find

../src/esp32/ESP32CryptoEngine.cpp:4:10: fatal error: crypto/includes.h: No such file or directory

[mkinney]

I've created a branch: https://github.com/mkinney/Meshtastic-device/tree/poc_unit_testing

If you go into test2, you should be able to run "make".

From main directory, you should be able to run "pio test".

Neither will work, but at least you can see what I'm seeing.

[mkinney]

The pio test stuff is probably the only way this would work. Need to spend some time looking into it.

[mkinney]

I think using the test_build_project_src = true might be the way to go as described here
https://docs.platformio.org/en/latest/plus/unit-testing.html

However, I don't like this warning:

Please note that you will need to use #ifdef UNIT_TEST and #endif guard to hide non-test related source code. For example, own main() or setup() / loop() functions.

[mkinney]

This might give us code coverage: https://mber.dev/software/platformio-unit-test-coverage

[ajmcquilkin]

Are unit tests in this repo still a priority in this form? Happy to poke around if that's helpful

[garthvh]

Are unit tests in this repo still a priority in this form? Happy to poke around if that's helpful

Yes!

[ajmcquilkin]

Following up on this conversation. From my research, it seems like the most effective (robust, repeatable) way to unit test the firmware code is to break the code up into relatively small local libraries in a /lib directory. This would then register each subdirectory of the /lib directory as a local library, which could then be tested individually. This is the recommended approach in this document from the PlatformIO team.

This testing would need to be done in the env:native environment, since testing in a hardware environment attempts to upload the code to the device to test on metal. Alternatively, PlatformIO supports chip simulators, which could be run within a CI/CD flow. I personally view these two approaches as complementary, where library code is tested in the env:native environment and where integration tests are run in a chip sim.

Assuming we choose to move forward with code refactoring into /lib, there would take a non-negligible amount of required work before any unit tests could be written. This being said, this refactoring work could in theory be done over any number of tickets.

For testing specifics, it seems like the Unity testing framework is the simplest and most robust option due to built-in support in PlatformIO, but I don't see an issue with using GoogleTest either.

This is just my initial dive into PlatformIO testing, would love thoughts on any/all of this!

Edit: The migration to the /lib directory would only need to be for code that doesn't have any hardware dependencies. Code that depends on hardware devices can in theory stay within the /src directory, although this would then require integration testing to validate this functionality.

[thebentern]

Assuming we choose to move forward with code refactoring into /lib, there would take a non-negligible amount of required work before any unit tests could be written. This being said, this refactoring work could in theory be done over any number of tickets.

This is correct. I ran into this with trying to port a stripped down repeater version of the device firmware. My suggestion would be to take the least "side-effecty" and containing the transitive dependencies to start with. If you want to help take on this effort, creating some small proof of concept unit tests on some simple static method classes would be great. It'll be a good place to build on if we have an example folks can look at.
Of course, as you said. Those files will need to be moved into the lib/ directory, but I think we can do that as needed. That's why I recommend trying to start at the bottom of the dependency graph and work your way up. Otherwise you end up having to lift half of the codebase over there, which we want eventually... It's just a little too monumental of a task to start with.

For testing specifics, it seems like the Unity testing framework is the simplest and most robust option due to built-in support in PlatformIO, but I don't see an issue with using GoogleTest either.

Unity is definitely the way to go, IMO. It's simple and integrates better with PIO.