For the purposes of this tutorial, we'll be setting up an environment in Docker. Docker is handy because it's fairly portable, and won't interfere with the rest of your machine setup. It also offers a strong guarantee that you're testing the same way that others working on the project are. Finally, we can get away with using the same Docker image that's run by the OpenBMC continuous integration bot, so we have even more confidence that we're running relevant tests the way they'd be run upstream.
Installation of Docker CE (Community Edition) varies by platform, and may differ in your organization. But the Docker Docs are a good place to start looking.
Check that the installation was successful by running sudo docker run hello-world.
You'll want a couple of different repositories, so start by making a place for it all to go, and clone the CI scripts:
mkdir openbmc-ci-tests cd openbmc-ci-tests git clone https://github.com/openbmc/openbmc-build-scripts.git
phosphor-host-ipmidWe also need to put a copy of the project you want to test against here. But you've probably got a copy checked out already, so we're going to make a git worktree. You can read more about those by running git help worktree, but the basic idea is that it's like having a second copy of your repo - but the second copy is in sync with your main copy, knows about your local branches, and protects you from checking out the same branch in two places.
Your new worktree doesn't know about any untracked files you have in your main worktree, so you should get in the habit of committing everything you want to run tests against. However, because of the implementation of run-unit-test-docker.sh, you can't run the CI with untracked changes anyways, so this isn't the worst thing in the world. (If you make untracked changes in your testing worktree, it's easy to update a commit with those.)
Note the placeholders in the following steps; modify the commands to match your directory layout.
cd /my/dir/for/phosphor-host-ipmid git worktree add /path/to/openbmc-ci-tests/phosphor-host-ipmid
Now, if you cd /path/to/openbmc-ci-tests, you should see a directory phosphor-host-ipmid/, and if you enter it and run git status you will see that you're likely on a new branch named phosphor-host-ipmid. This is just for convenience, since you can't check out a branch in your worktree that's already checked out somewhere else; you can safely ignore or delete that branch later.
However, Git won't be able to figure out how to get to your main worktree (/my/dir/for/phosphor-host-ipmid), so we'll need to mount it when we run. Open up /path/to/openbmc-ci-tests/openbmc-build-scripts/run-unit-test-docker.sh and find where we call docker run, way down at the bottom. Add an additional argument, remembering to escape the newline (''):
PHOSPHOR_IPMI_HOST_PATH="/my/dir/for/phosphor-host-ipmid" docker run --blah-blah-existing-flags \ -v ${PHOSPHOR_IPMI_HOST_PATH}:${PHOSPHOR_IPMI_HOST_PATH} \ -other \ -args
Then commit this, so you can make sure not to lose it if you update the scripts repo:
cd openbmc-build-scripts git add run-unit-test-docker.sh git commit -m "mount phosphor-host-ipmid"
NOTE: There are other ways to do this besides a worktree; other approaches trade the cruft of mounting extra paths to the Docker container for different cruft:
You can create a local upstream:
cd openbmc-ci-tests mkdir phosphor-host-ipmid cd phosphor-host-ipmid git init cd /my/dir/for/phosphor-host-ipmid git remote add /path/to/openbmc-ci-tests/phosphor-host-ipmid ci git push ci
This method would require you to push your topic branch to ci and then git checkout the appropriate branch every time you switched topics:
cd /my/dir/for/phosphor-host-ipmid git commit -m "my changes to be tested" git push ci cd /path/to/openbmc-ci-tests/phosphor-host-ipmid git checkout topic-branch
You can also create a symlink from your Git workspace into openbmc-ci-tests/. This is especially not recommended, since you won't be able to work on your code in parallel while the tests run, and since the CI scripts are unhappy when you have untracked changes - which you're likely to have during active development.
The OpenBMC CI scripts take care of the build for you, and run the test suite. Build and run like so:
sudo WORKSPACE=$(pwd) UNIT_TEST_PKG=phosphor-host-ipmid \ ./openbmc-build-scripts/run-unit-test-docker.sh
The first run will take a long time! But afterwards it shouldn't be so bad, as many parts of the Docker container are already downloaded and configured.
Your results will appear in openbmc-ci-tests/phosphor-host-ipmid/test/test-suite.log, as well as being printed to stdout. You will also see other .log files generated for each test file, for example sample_unittest.log. All these *.log files are human-readable and can be examined to determine why something failed
Now that you've got an easy working environment for running tests, let's write some new ones.
In /my/dir/for/phosphor-host-ipmid, create a new branch based on master (or your current patchset). For this tutorial, let's call it sensorhandler-tests.
git checkout -b sensorhandler-tests master
This creates a new branch sensorhandler-tests which is based on master, then checks out that branch for you to start hacking.
For this tutorial, we'll be adding some basic unit testing of the struct accessors for get_sdr::GetSdrReq, just because they're fairly simple. The text of the struct and accessors is recreated here:
/** * Get SDR */ namespace get_sdr { struct GetSdrReq { uint8_t reservation_id_lsb; uint8_t reservation_id_msb; uint8_t record_id_lsb; uint8_t record_id_msb; uint8_t offset; uint8_t bytes_to_read; } __attribute__((packed)); namespace request { inline uint8_t get_reservation_id(GetSdrReq* req) { return (req->reservation_id_lsb + (req->reservation_id_msb << 8)); }; inline uint16_t get_record_id(GetSdrReq* req) { return (req->record_id_lsb + (req->record_id_msb << 8)); }; } // namespace request ... } // namespace get_sdr
We'll create the tests in test/sensorhandler_unittest.cpp; go ahead and start that file with your editor.
First, include the header you want to test, as well as the GTest header:
#include <sensorhandler.hpp> #include <gtest/gtest.h>
Let's plan the test cases we care about before we build any additional scaffolding. We've only got two functions - get_reservation_id() and get_record_id(). We want to test:
For get_reservation_id():
TEST(SensorHandlerTest, GetSdrReq_get_reservation_id_HappyPath) { } TEST(SensorHandlerTest, GetSdrReq_get_reservation_id_NullInputDies) { } TEST(SensorHandlerTest, GetSdrReq_get_reservation_id_Uint16MaxWorksCorrectly) { }
For get_record_id(), we have pretty much the same set of tests:
TEST(SensorHandlerTest, GetSdrReq_get_record_id_HappyPath) { } TEST(SensorHandlerTest, GetSdrReq_get_record_id_NullInputDies) { } TEST(SensorHandlerTest, GetSdrReq_get_record_id_Uint16MaxWorksCorrectly) { }
In the case of these two methods, there's really not much else to test. Some types of edge cases - like overflow/underflow - are prevented by C++'s strong typing; other types - like passing the incorrect type - are impossible to insulate against because it's possible to cast anything to a GetSdrReq* if we want. Since these are particularly boring, they make a good example for a tutorial like this; in practice, tests you write will likely be for more complicated code! We'll talk more about this in the Best Practices section below.
Let's implement the get_reservation_id() items first. The implementations for get_record_id() will be identical, so we won't cover them here.
For the happy path, we want to make it very clear that the test value we're using is within range, so we express it in binary. We also want to be able to ensure that the MSB and LSB are being combined in the correct order, so we make sure that the MSB and LSB values are different (don't use 0x3333 as the expected ID here). Finally, we want it to be obvious to the reader if we have populated the GetSdrReq incorrectly, so we've labeled all the fields. Since we are only testing one operation, it's okay to use either ASSERT_EQ or EXPECT_EQ; more on that in the Best Practices section.
TEST(SensorHandlerTest, GetSdrReq_get_reservation_id_HappyPath) { uint16_t expected_id = 0x1234; // Expected ID spans both bytes. GetSdrReq input = {0x34, // Reservation ID LSB 0x12, // Reservation ID MSB 0x00, // Record ID LSB 0x00, // Record ID MSB 0x00, // Offset 0x00}; // Bytes to Read uint16_t actual = get_sdr::request::get_reservation_id(&input); ASSERT_EQ(actual, expected_id); }
We don't expect that our GetSdrReq pointer will ever be null; in this case, the null pointer validation is done much, much earlier. So it's okay for us to specify that in the unlikely case we're given a null pointer, we die. We don't really care what the output message is.
TEST(SensorHandlerTest, GetSdrReq_get_reservation_id_NullInputDies) { ASSERT_DEATH(get_sdr::request::get_reservation_id(nullptr), ".*"); }
Finally, while negative values are taken care of by C++'s type system, we can at least check that our code still works happily with UINT16_MAX. This test is similar to the happy path test, but uses an edge value instead.
TEST(SensorHandlerTest, GetSdrReq_get_reservation_id_Uint16MaxWorksCorrectly) { uint16_t expected_id = 0xFFFF; // Expected ID spans both bytes. GetSdrReq input = {0xFF, // Reservation ID LSB 0xFF, // Reservation ID MSB 0x00, // Record ID LSB 0x00, // Record ID MSB 0x00, // Offset 0x00}; // Bytes to Read uint16_t actual = get_sdr::request::get_reservation_id(&input); ASSERT_EQ(actual, expected_id); }
The get_record_id() tests are identical, except that they are testing the Record ID field. They will not be duplicated here.
Finally, we'll need to add the new tests to test/Makefile.am. You can mimic other existing test setups:
# Build/add sensorhandler_unittest to test suite sensorhandler_unittest_CPPFLAGS = \ -Igtest \ $(GTEST_CPPFLAGS) \ $(AM_CPPFLAGS) sensorhandler_unittest_CXXFLAGS = \ $(PTHREAD_CFLAGS) \ $(CODE_COVERAGE_CXXFLAGS) \ $(CODE_COVERAGE_CFLAGS) \ -DBOOST_COROUTINES_NO_DEPRECATION_WARNING sensorhandler_unittest_LDFLAGS = \ -lgtest_main \ -lgtest \ -pthread \ $(OESDK_TESTCASE_FLAGS) \ $(CODE_COVERAGE_LDFLAGS) sensorhandler_unittest_SOURCES = \ %reldir%/sensorhandler_unittest.cpp check_PROGRAMS += %reldir%/sensorhandler_unittest
Commit your test changes. Then, you'll want to checkout the sensorhandler-tests branch in your CI worktree, which will involve releasing it from your main worktree:
cd /my/dir/for/phosphor-host-ipmid git add test/sensorhandler_unittest.cpp git commit -s git checkout master # Here you can use any branch except sensorhandler-tests cd /path/to/openbmc-ci-tests/phosphor-host-ipmid git checkout sensorhandler-tests
Now you can run the test suite as described earlier in the document. If you see a linter error when you run, you can actually apply the cleaned-up code easily:
cd ./phosphor-host-ipmid git diff # Examine the proposed changes git add -u # Apply the proposed changes git commit --amend
(If you will need to apply the proposed changes to multiple commits, you can do this with interactive rebase, which won't be described here.)
While a good unit test can ensure your code's stability, a flaky or poorly-written unit test can make life harder for contributors down the road. Some things to remember:
Include both positive (happy-path) and negative (error) testing in your testbench. It's not enough to know that the code works when it's supposed to; we also need to know that it fails gracefully when something goes wrong. Applying edge-case testing helps us find bugs that may take years to occur (and reproduce!) in the field.
Keep your tests small. Avoid branching - if you feel a desire to, instead explore that codepath in another test. The best tests are easy to read and understand.
When a test fails, it's useful if the test is named in such a way that you can tell what it's supposed to do and when. That way you can be certain whether the change you made really broke it or not. A good pattern is Object_NameCircumstanceResult - for example, FooFactory_OutOfBoundsNameThrowsException. From the name, it's very clear that when some "name" is out of bounds, an exception should be thrown. (What "name" is should be clear from the context of the function in FooFactory.)
Don't test other people's code. Make sure to limit the test assertions to the code under test. For example, don't test what happens if you give a bad input to sdbusplus when you're supposed to be testing phosphor-host-ipmid.
However, don't trust other people's code, either! Try to test how you respond when a service fails unexpectedly. Rather than checking if sdbusplus fails on bad inputs, check whether you handle an sdbusplus failure gracefully. You can use GMock for this kind of testing.
Think about testing when you write your business logic code. Concepts like dependency injection and small functions make your code more testable - you'll be thanking yourself later when you're writing tests.
Finally, you're very likely to find bugs while writing tests, especially if it's for code that wasn't previously unit-tested. It's okay to check in a bugfix along with a test that verifies the fix worked, if you're only doing one test and one bugfix. If you're checking in a large suite of tests, do the bugfixes in independent commits which your test suite commit is based on:
master -> fix Foo.Abc() -> fix Foo.Def() -> Fix Foo.Ghi() -> test Foo class
You can send your test update and any associated bugfixes for review to Gerrit as you would send any other change. For the Tested: field in the commit message, you can state that you ran the new unit tests written.
Tests are written primarily to be read. So when you review a test, you're the first customer of that test!
First, all the best practices listed above for writing tests are things you should check for and encourage when you're reading tests.
Next, you should ensure that you can tell what's going on when you read the test. If it's not clear to you, it's not going to be clear to someone else, and the test is more prone to error - ask!
Finally, think about what's not being tested. If there's a case you're curious about and it isn't covered, you should mention it to the committer.
Now that you've got a handy setup as described earlier in this document, you can quickly download and run the tests yourself. Within the Gerrit change, you should be able to find a button that says "Download", which will give you commands for various types of downloads into an existing Git repo. Use "Checkout", for example:
cd openbmc-ci-tests/phosphor-host-ipmid git fetch "https://gerrit.openbmc-project.xyz/openbmc/phosphor-host-ipmid" \ refs/changes/43/23043/1 && git checkout FETCH_HEAD
This won't disturb the rest of your Git repo state, and will put your CI worktree into detached-HEAD mode pointing to the commit that's under review. You can then run your tests normally, and even make changes and push again if the commit was abandoned or otherwise left to rot by its author.
Doing so can be handy in a number of scenarios:
Thanks very much to Patrick Venture for his prior work putting together documentation on this topic internal to Google.