# Running Tests ## Setting Up Your Environment 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. ### Install Docker Installation of Docker CE (Community Edition) varies by platform, and may differ in your organization. But the [Docker Docs](https://docs.docker.com/v17.12/install) are a good place to start looking. Check that the installation was successful by running `sudo docker run hello-world`. ### Download OpenBMC Continuous Integration Image You'll want a couple of different repositories, so start by making a place for it all to go, and clone the CI scripts: ```shell mkdir openbmc-ci-tests cd openbmc-ci-tests git clone https://github.com/openbmc/openbmc-build-scripts.git ``` ### Add `phosphor-host-ipmid` We 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. ```shell 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 ('\'): ```shell 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: ```shell 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: ```shell 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: ```shell 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. ## Building and Running The OpenBMC CI scripts take care of the build for you, and run the test suite. Build and run like so: ```shell 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. ## Reading Output 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 # Writing Tests Now that you've got an easy working environment for running tests, let's write some new ones. ## Setting Up Your Environment 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`. ```shell 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. ## Write Some Tests 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: ```c++ /** * 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: ```c++ #include #include ``` 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: - "Happy path" - in an ideal case, everything works correctly - Error handling - when given bad input, things break as expected - Edge cases - when given extremes (e.g. very large or very small numbers), things work correctly or break as expected For `get_reservation_id()`: ```c++ 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: ```c++ 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. ```c++ 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. ```c++ 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. ```c++ 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: ```make # 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 ``` ## Run the New Tests 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: ```shell 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: ```shell 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.) ## Best Practices 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 ## Sending for Review 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. # Reviewing Tests Tests are written primarily to be read. So when you review a test, you're the first customer of that test! ## Best Practices 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. ## Quickly Running At Home 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: ```shell 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: - Jenkins isn't responding - The Jenkins build is broken for a reason beyond the committer's control - The committer doesn't have "Ok-To-Test" permission, and you don't have permission to grant it to them # Credits Thanks very much to Patrick Venture for his prior work putting together documentation on this topic internal to Google.