From 00a553df9c0e01f01cd6761fcd9e7eb390545cd4 Mon Sep 17 00:00:00 2001 From: Emily Shaffer Date: Tue, 25 Jun 2019 17:59:09 -0700 Subject: doc: add writing test chapter to testing.md Add a tutorial on writing a new test, based on writing unit tests for sensorhandler.hpp - specifically GetSdrReq, as it's a good candidate for unit testing. Included is a nonexhaustive list of testing best practices. More best practices are always welcome. Tested: Viewed testing.md with Chrome builtin Markdown interpreter Change-Id: I418af8f972a5a0ff36b786cda6fdf9b6b308e8d5 Signed-off-by: Emily Shaffer --- docs/testing.md | 279 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 279 insertions(+) (limited to 'docs/testing.md') diff --git a/docs/testing.md b/docs/testing.md index 6688912..cd48620 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -137,12 +137,291 @@ 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 ## Best Practices -- cgit v1.2.1