1
\$\begingroup\$

In an Arduino project, I'm using some code which is a slightly modified version of a public library. I made several modifications to it:

  • I performed some minor changes I needed to make the code work.
  • Instead of relying on Arduino library, it makes use of the wrappers, which can be mocked in the tests.
  • I started adding the actual tests of the class.

When writing the tests, I was quite surprised by their complexity—and how unreadable and unmaintainable they are. The tested code itself doesn't seem that complicated: granted, there are multiple possible branches, but it still is relatively basic. Each test of a given execution path seems however overly complex, and doesn't translate much of what the code is doing. I'm pretty sure that when I will found those tests six months later, I would have no idea what they are doing, and shall spend a lot of time trying to figure out how they work. Similarly, any change in the actual code would lead to painful changes in the tests.

Here they are. At least the first five tests I wrote—a few dozens more need to be written in order to cover all the possible code execution paths. The last two tests are particularly illustrative.

What can be done to make them more bearable? Am I doing it completely wrong? Should I be implementing some sort of DSL just for the tests?

#include "../src/wrappers/AHT21WrapperArduino.h"
#include "../src/wrappers/WireWrapper.h"
#include <gmock/gmock.h>
#include <gtest/gtest.h>

using ::testing::Return;

const int8_t StatusNone = 0b00000000;
const int8_t StatusCalibrated = 0b00001000;
const int8_t StatusBusy = 0b10000000;

class MockTime: public Time {
 public:
    MOCK_METHOD(long, now, (), (override));
    MOCK_METHOD(void, pause, (unsigned long milliseconds), (override));
};

class MockWire: public WireWrapper {
 public:
    MOCK_METHOD(void, begin, (), (override));
    MOCK_METHOD(void, beginTransmission, (uint8_t address), (override));
    MOCK_METHOD(uint8_t, endTransmission, (), (override));
    MOCK_METHOD(uint8_t, requestFrom, (uint8_t address, uint8_t quantity), (override));
    MOCK_METHOD(int, available, (), (override));
    MOCK_METHOD(int, read, (), (override));
    MOCK_METHOD(size_t, write, (uint8_t data), (override));
};

TEST(AHT21WrapperArduino, Init) {
    // Standard initialization, where everything goes as expected.
    //
    // Expecting calls:
    //  wire.begin()
    //  wire.beginTransmission(0x38)
    //  wire.endTransmission() -> 0
    //  time.pause(40)
    //  wire.requestFrom(0x38, 0x01)
    //  wire.available() -> 1
    //  wire.read() -> 0b00003000  Device calibrated.
    MockTime time;
    MockWire wire;

    EXPECT_CALL(wire, begin());
    EXPECT_CALL(wire, beginTransmission(0x38));
    EXPECT_CALL(wire, endTransmission()).WillOnce(Return(0));
    EXPECT_CALL(wire, requestFrom(0x38, 0x01));
    EXPECT_CALL(wire, available()).WillOnce(Return(1));
    EXPECT_CALL(wire, read()).WillOnce(Return(StatusCalibrated));
    EXPECT_CALL(time, pause(40));

    auto aht21 = AHT21WrapperArduino(time, wire);
    auto result = aht21.init();
    ASSERT_EQ(0, result);
}

TEST(AHT21WrapperArduino, InitNotConnected) {
    // If the device refuses to connect, the init method returns 1.
    //
    // Expecting calls:
    //  wire.begin()
    //  wire.beginTransmission(0x38)
    //  wire.endTransmission() -> 1
    //  wire.beginTransmission(0x38)
    //  wire.endTransmission() -> 1
    MockTime time;
    MockWire wire;

    EXPECT_CALL(wire, begin());
    EXPECT_CALL(wire, beginTransmission(0x38)).Times(2);
    EXPECT_CALL(wire, endTransmission()).Times(2).WillRepeatedly(Return(1));
    EXPECT_CALL(time, pause(20));

    auto aht21 = AHT21WrapperArduino(time, wire);
    auto result = aht21.init();
    ASSERT_EQ(1, result);
}

TEST(AHT21WrapperArduino, InitConnectionSecondChance) {
    // One should give a second chance when the first connection fails.
    //
    // Expecting calls:
    //  wire.begin()
    //  wire.beginTransmission(0x38)
    //  wire.endTransmission() -> 1
    //  time.pause(20)
    //  wire.beginTransmission(0x38)
    //  wire.endTransmission() -> 0
    //  time.pause(40)
    //  wire.requestFrom(0x38, 0x01)
    //  wire.available() -> 1
    //  wire.read() -> 0b00003000  Device calibrated.
    MockTime time;
    MockWire wire;

    EXPECT_CALL(wire, begin());
    EXPECT_CALL(wire, beginTransmission(0x38)).Times(2);
    EXPECT_CALL(wire, endTransmission())
        .Times(2)
        .WillOnce(Return(1))
        .WillOnce(Return(0));
    EXPECT_CALL(wire, requestFrom(0x38, 0x01));
    EXPECT_CALL(wire, available()).WillOnce(Return(1));
    EXPECT_CALL(wire, read()).WillOnce(Return(StatusCalibrated));
    EXPECT_CALL(time, pause(20));
    EXPECT_CALL(time, pause(40));

    auto aht21 = AHT21WrapperArduino(time, wire);
    auto result = aht21.init();
    ASSERT_EQ(0, result);
}

TEST(AHT21WrapperArduino, InitCalibrate) {
    // The device should be calibrated, if needed.
    //
    // Expecting calls:
    //  wire.begin()
    //  wire.beginTransmission(0x38)
    //  wire.endTransmission() -> 0
    //  time.pause(40)
    //  wire.requestFrom(0x38, 0x01)
    //  wire.available() -> 1
    //  wire.read() -> 0b00000000  Device not calibrated.
    //  wire.beginTransmission(0x38)
    //  wire.write(0xBE)
    //  wire.write(0x80)
    //  wire.write(0x00)
    //  wire.endTransmission() -> 0
    //  wire.beginTransmission(0x38)
    //  wire.write(0xAC)
    //  wire.write(0x30)
    //  wire.write(0x00)
    //  wire.endTransmission() -> 0
    //  time.pause(75)
    //  wire.available() -> 1
    //  wire.read() -> 0b00001000  Device not busy.
    //  wire.available() -> 1
    //  wire.read() -> 0b00001000  Device calibrated.
    MockTime time;
    MockWire wire;

    EXPECT_CALL(wire, begin());
    EXPECT_CALL(wire, beginTransmission(0x38)).Times(3);
    EXPECT_CALL(wire, endTransmission()).Times(3).WillRepeatedly(Return(0));
    EXPECT_CALL(wire, requestFrom(0x38, 0x01)).Times(3);
    EXPECT_CALL(wire, available()).Times(3).WillRepeatedly(Return(1));
    EXPECT_CALL(wire, read()).Times(3)
        .WillOnce(Return(0))  // Device not calibrated.
        .WillOnce(Return(StatusCalibrated))  // Device not busy.
        .WillOnce(Return(StatusCalibrated));  // Device calibrated now.
    EXPECT_CALL(wire, write(0xBE));
    EXPECT_CALL(wire, write(0x80));
    EXPECT_CALL(wire, write(0xAC));
    EXPECT_CALL(wire, write(0x30));
    EXPECT_CALL(wire, write(0x00)).Times(2);
    EXPECT_CALL(time, pause(40));
    EXPECT_CALL(time, pause(75));

    auto aht21 = AHT21WrapperArduino(time, wire);
    auto result = aht21.init();
    ASSERT_EQ(0, result);
}

TEST(AHT21WrapperArduino, InitBusy) {
    // If the device is busy, the initialization fails.
    //
    // Expecting calls:
    //  wire.begin()
    //  wire.beginTransmission(0x38)
    //  wire.endTransmission() -> 0
    //  time.pause(40)
    //  wire.requestFrom(0x38, 0x01)
    //  wire.available() -> 1
    //  wire.read() -> 0b00000000  Device not calibrated.
    //  wire.beginTransmission(0x38)
    //  wire.write(0xBE)
    //  wire.write(0x80)
    //  wire.write(0x00)
    //  wire.endTransmission() -> 0
    //  wire.beginTransmission(0x38)
    //  wire.write(0xAC)
    //  wire.write(0x30)
    //  wire.write(0x00)
    //  wire.endTransmission() -> 0
    //  time.pause(75)
    //  wire.available() -> 1     ┓ 1
    //  wire.read() -> 0b10000000 ┃ Device busy.
    //  time.pause(1)             ┛
    //  wire.available() -> 1     ┓ 2
    //  wire.read() -> 0b10000000 ┃ Device busy.
    //  time.pause(1)             ┛
    //  ...                       ┓ n
    //  ...                       ┃
    MockTime time;
    MockWire wire;

    EXPECT_CALL(wire, begin());
    EXPECT_CALL(wire, beginTransmission(0x38)).Times(3);
    EXPECT_CALL(wire, endTransmission()).Times(3).WillRepeatedly(Return(0));
    EXPECT_CALL(wire, requestFrom(0x38, 0x01)).Times(103);
    EXPECT_CALL(wire, available()).Times(103).WillRepeatedly(Return(1));
    EXPECT_CALL(wire, read()).Times(103)
        .WillOnce(Return(0))  // Device not calibrated.
        .WillRepeatedly(Return(StatusBusy));
    EXPECT_CALL(wire, write(0xBE));
    EXPECT_CALL(wire, write(0x80));
    EXPECT_CALL(wire, write(0xAC));
    EXPECT_CALL(wire, write(0x30));
    EXPECT_CALL(wire, write(0x00)).Times(2);
    EXPECT_CALL(time, pause(40));
    EXPECT_CALL(time, pause(75));
    EXPECT_CALL(time, pause(1)).Times(102);

    auto aht21 = AHT21WrapperArduino(time, wire);
    auto result = aht21.init();
    ASSERT_EQ(2, result);
}

For the sake of completeness, here's the source code under test. Note, however, that the question is not about this piece of code, but rather the test code above.

#include "AHT21WrapperArduino.h"

#ifdef DEBUG_TESTS
#include <iostream>
#include <bitset>
#endif

AHT21WrapperArduino::AHT21WrapperArduino(Time &time, WireWrapper &wire) :
    time { time },
    wire { wire } { }

int AHT21WrapperArduino::init() {
    #ifdef DEBUG_TESTS
    std::cout << "Started the initialization of the device." << std::endl;
    #endif
    this->wire.begin();
    if (!this->isConnected()) {
        return 1;
    }

    this->time.pause(40);

    if (!this->isCalibrated()) {
        #ifdef DEBUG_TESTS
        std::cerr << "The device is not calibrated yet." << std::endl;
        #endif
        this->initialize();
        this->measure();
        this->time.pause(75);
        uint8_t counter = 0;
        while (this->isBusy()) {
            this->time.pause(1);
            if (counter > 100) {
                return 2;  // Give up after 100 ms.
            }

            ++counter;
        }

        if (!this->isCalibrated()) {
            return 3;
        }
    }

    #ifdef DEBUG_TESTS
    std::cerr << "Initialization finished." << std::endl;
    #endif
    return 0;
}

int AHT21WrapperArduino::read(float* temperature, float* humidity) {
    if (!this->available()) {
        return 1;
    }

    this->measure();
    this->time.pause(75);
    uint8_t counter = 0;
    while (this->isBusy()) {
        this->time.pause(1);
        if (counter > 100) {
            return 2;  // Give up after 100 ms.
        }

        ++counter;
    }

    this->readData();

    *temperature = ((float)temperatureData / 1048576) * 200 - 50;
    *humidity = ((float)humidityData / 1048576) * 100;
    return 0;
}

bool AHT21WrapperArduino::isConnected() {
    #ifdef DEBUG_TESTS
    std::cerr << "Determining if the device is connected." << std::endl;
    #endif
    this->wire.beginTransmission(this->deviceAddress);
    if (this->wire.endTransmission() == 0) {
        #ifdef DEBUG_TESTS
        std::cerr << "The device is connected." << std::endl;
        #endif
        return true;
    }

    #ifdef DEBUG_TESTS
    std::cerr << "Giving the device a second chance." << std::endl;
    #endif
    // If IC failed to respond, give it 20ms more for Power On Startup.
    // See datasheet, page 7.
    this->time.pause(20);

    this->wire.beginTransmission(this->deviceAddress);
    return this->wire.endTransmission() == 0;
}

uint8_t AHT21WrapperArduino::getStatus() {
    this->wire.requestFrom(this->deviceAddress, (uint8_t)1);
    if (this->wire.available()) {
        auto status = this->wire.read();
        #ifdef DEBUG_TESTS
        auto statusBits = std::bitset<8>(status);
        std::cerr << "The status is 0b" << statusBits << "." << std::endl;
        #endif
        return status;
    }

    return 0;
}

bool AHT21WrapperArduino::isCalibrated() {
    #ifdef DEBUG_TESTS
    std::cerr << "Determining if the device is calibrated." << std::endl;
    #endif
    return this->getStatus() & (1 << 3);
}

bool AHT21WrapperArduino::isBusy() {
    #ifdef DEBUG_TESTS
    std::cerr << "Determining if the device is busy." << std::endl;
    #endif
    return this->getStatus() & (1 << 7);
}

bool AHT21WrapperArduino::initialize() {
    #ifdef DEBUG_TESTS
    std::cerr << "Initializing the device." << std::endl;
    #endif
    this->wire.beginTransmission(this->deviceAddress);
    this->wire.write(0xBE);  // Initialize.
    this->wire.write(0x80);
    this->wire.write(0x00);
    return this->wire.endTransmission() == 0;
}

bool AHT21WrapperArduino::measure() {
    this->wire.beginTransmission(this->deviceAddress);
    this->wire.write(0xAC);  // Measure.
    this->wire.write(0x30);
    this->wire.write(0x00);
    return this->wire.endTransmission() == 0;
}

bool AHT21WrapperArduino::available() {
    if (this->isBusy()) {
        return false;
    }

    this->readData();
    return true;
}

void AHT21WrapperArduino::readData() {
    if (this->wire.requestFrom(this->deviceAddress, (uint8_t)6) > 0) {
        this->wire.read();
        uint32_t incoming = 0;
        incoming |= (uint32_t)this->wire.read() << (8 * 2);
        incoming |= (uint32_t)this->wire.read() << (8 * 1);
        uint8_t midByte = this->wire.read();
        incoming |= midByte;

        this->humidityData = incoming >> 4;
        this->temperatureData = (uint32_t)midByte << (8 * 2);
        this->temperatureData |= (uint32_t)this->wire.read() << (8 * 1);
        this->temperatureData |= (uint32_t)this->wire.read() << (8 * 0);
        this->temperatureData = this->temperatureData & ~(0xFFF00000);
    } else {
        this->humidityData = 0;
        this->temperatureData = 0;
    }
}
```
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

I think the last test case is illustrative because you've repeated in comments all the stuff that should have been in the code. Instead of:

TEST(AHT21WrapperArduino, InitBusy) {
    // If the device is busy, the initialization fails.

try:

TEST(AHT21WrapperArduino, IfDeviceBusyThenInitFails) {

Instead of:

// Expecting calls:
//  wire.begin()
//  wire.beginTransmission(0x38)
//  wire.endTransmission() -> 0
//  time.pause(40)
//  wire.requestFrom(0x38, 0x01)
//  wire.available() -> 1
//  wire.read() -> 0b00000000  Device not calibrated.
//  wire.beginTransmission(0x38)
//  wire.write(0xBE)

try:

InSequence s;
EXPECT_CALL(wire, begin());
EXPECT_CALL(wire, beginTransmission(0x38));
EXPECT_CALL(wire, endTransmission()).WillOnce(Return(0));
EXPECT_CALL(time, pause(40));
EXPECT_CALL(wire, requestFrom(0x38, 0x01));
EXPECT_CALL(wire, available()).WillOnce(Return(1));
EXPECT_CALL(wire, read()).WillOnce(Return(0));  // Device not calibrated
EXPECT_CALL(wire, beginTransmission(0x38));
EXPECT_CALL(wire, write(0xBE));

and so on. (Admittedly I'm not sure whether you need a bunch of .RetiresOnSaturation sprinkled in there.)


If you find yourself typing the same things over and over, consider pulling out the repeated code into a helper function. For example:

TEST(AHT21WrapperArduino, IfDeviceBusyThenInitFails) {
    MockTime time;
    MockWire wire;
    auto aht21 = AHT21WrapperArduino(time, wire);

    InSequence s;
    expectTransmission(wire, 0x38, {});
    EXPECT_CALL(time, pause(40));
    EXPECT_CALL(wire, requestFrom(0x38, 0x01));
    EXPECT_CALL(wire, available()).WillOnce(Return(1));
    EXPECT_CALL(wire, read()).WillOnce(Return(0));  // Device not calibrated
    expectTransmission(wire, 0x38, {0xBE, 0x80, 0x00});
    expectTransmission(wire, 0x38, {0xAC, 0x30, 0x00});
    EXPECT_CALL(time, pause(75));
    EXPECT_CALL(wire, available()).WillOnce(Return(1));

etc., where expectTransmission is just a C++ function

static void expectTransmission(MockWire& wire, int deviceAddress,
                               std::vector<int> bytes) {
    EXPECT_CALL(wire, beginTransmission(0x38));
    for (int i : bytes) {
        EXPECT_CALL(wire, write(i));
    }
    EXPECT_CALL(wire, endTransmission()).WillOnce(Return(0));
}

Other than this basic stuff, I don't think there's much to improve here.

\$\endgroup\$
2
  • \$\begingroup\$ The problem is, GMock doesn't work this way. When two or more EXPECT_CALLs apply to the same method, only the last one is actually used, which means that they have to be combined. It also means that any pulling of common code in common functions could be tricky: I could miss the fact that an EXPECT_CALL occurs in multiple of such functions. \$\endgroup\$ Commented Jul 6, 2021 at 8:07
  • 1
    \$\begingroup\$ My impression is that InSequence modifies those rules; see e.g. stackoverflow.com/questions/52765902/… . I admit I haven't tested my suggestion... but did you test it? :) \$\endgroup\$ Commented Jul 6, 2021 at 23:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.