diff options
Diffstat (limited to 'docs')
| -rw-r--r-- | docs/configuration.md | 2 | ||||
| -rw-r--r-- | docs/contributing.md | 122 | ||||
| -rw-r--r-- | docs/testing.md | 471 |
3 files changed, 594 insertions, 1 deletions
diff --git a/docs/configuration.md b/docs/configuration.md index 61caaf1..4e833d9 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1,4 +1,4 @@ -#Device ID Configuration# +# Device ID Configuration There is a default dev_id.json file provided by meta-phosphor/common/recipes-phosphor/ipmi/phosphor-ipmi-host.bb diff --git a/docs/contributing.md b/docs/contributing.md new file mode 100644 index 0000000..8ccb3ec --- /dev/null +++ b/docs/contributing.md @@ -0,0 +1,122 @@ +# Contributing Guidelines + +This document attempts to outline some basic rules to follow when contributing +to OpenBMC's IPMI stack. It does *not* outline coding style; we follow the +[OpenBMC C++ style guide](https://github.com/openbmc/docs/blob/master/cpp-style-and-conventions) +for that. + +## Organizing Commits + +A good commit does exactly one thing. We prefer many small, atomic commits to +one large commit which makes many functional changes. + - Too large: "convert foo to new API; also fix CVE 1234 in bar" + - Too small: "move abc.h to top of include list" and "move xyz.h to bottom of + include list" + - Just right: "convert foo to new API" and "convert foo from tab to space" + +Often, creating small commits this way results in a number of commits which are +dependent on prior commits; Gerrit handles this situation well, so feel free to +push commits which are based on your change still in review. However, when +possible, your commit should stand alone on top of master - "Fix whitespace in +bar()" does not need to depend on "refactor foo()". Said differently, ensure +that topics which are not related to each other semantically are also not +related to each other in Git until they are merged into master. + +When pushing a stack of patches (current branch is >1 commits ahead of +origin/master), these commits will show up with that same relationship in +gerrit. This means that each patch must be merged in order of that relationship. +So if one of the patches in the middle needs to be changed, all the patches from +that point on would need to be pushed to maintain the relationship. This will +effectively rebase the unchanged patches, which would in turn trigger a new CI +build. Ideally, changes from the entire patchset could be done all in one go to +reduce unnecessary rebasing. + +When someone makes a comment on your commit in Gerrit, modify that commit and +send it again to Gerrit. This typically involves `git rebase --interactive` or +`git commit --amend`, for which there are many guides online. As mentioned in +the paragraph above, when possible you should make changes to multiple patches +in the same stack before you push, in order to minimize CI and notification +churn from the rebase operations. + +Commits which include changes that can be tested by a unit test should also +include a unit test to exercise that change, within the same commit. Unit tests +should be clearly written - even moreso than production code, unit tests are +meant primarily to be read by humans - and should test both good and bad +behaviors. Refer to the +[testing documentation](https://github.com/openbmc/phosphor-host-ipmid/blob/master/docs/testing.md) +for help writing tests, as well as best practices. + +## Formatting Commit Messages + +Your commit message should explain: + + - Concisely, *what* change are you making? e.g. "docs: convert from US to UK + spelling" This first line of your commit message is the subject line. + - Comprehensively, *why* are you making that change? In some cases, like a + small refactor, the why is fairly obvious. But in cases like the inclusion of + a new feature, you should explain why the feature is needed. + - Concisely, *how* did you test? This comes in the form of a "Tested:" footer + in your commit message and is required for all code changes in the IPMI + stack. It typically consists of copy-pasted ipmitool requests and responses. + When possible, use the high-level ipmitool commands (e.g. "ipmitool sensor + read 0x1"). In cases where that's not possible, or when testing edge or error + cases, it is acceptable to use "ipmitool raw" - but an explanation of your + output is appreciated. If the change can be validated entirely by running + unit tests, say so in the "Tested:" tag. + +Try to include the component you are changing at the front of your subject line; +this typically comes in the form of the class, module, handler, or directory you +are modifying. e.g. "apphandler: refactor foo to new API" + +Loosely, we try to follow the 50/72 rule for commit messages - that is, the +subject line should not exceed 50 characters and the body should not exceed 72 +characters. This is common practice in many projects which use Git. + +All commit messages must include a Signed-off-by line, which indicates that you +the contributor have agreed to the Developer Certificate of Origin. This line +must include the name you commonly use, often a given name and a family name or +surname. (ok: A. U. Thor, Sam Samuelsson, robert a. heinlein; not ok: +xXthorXx, Sam, RAH) + +## Sending Patches + +Like most projects in OpenBMC, we use Gerrit to review patches. Please check +the MAINTAINERS file to determine who needs to approve your review in order for +your change to be merged. Submitters will need to manually add their reviewers +in Gerrit; reviewers are not currently added automatically. Maintainers may not +see the commit if they have not been added to the review! + +## Pace of Review + +Contributors who are used to code reviews by their team internal to their own +company, or who are not used to code reviews at all, are sometimes surprised by +the pace of code reviews in open source projects. Try to keep in mind that those +reviewing your patch may be contributing to OpenBMC in a volunteer or +partial-time capacity, may be in a timezone far removed from your own, and may +have very deep review queues already of patches which have been waiting longer +than yours. + +If you feel your patch has been missed entirely, of course it's +alright to email the maintainers (addresses available in MAINTAINERS file) - but +a reasonable timeframe to do so is on the order of a week, not on the order of +hours. + +Additionally, the IPMI stack has a set of policies for when and how changes can +be approved; please check the MAINTAINERS file for the full list ("Change +approval rules"). + +The maintainers' job is to ensure that incoming patches are as correct and easy +to maintain as possible. Part of the nature of open source is attrition - +contributors can come and go easily - so maintainers tend not to put stock in +promises such as "I will add unit tests in a later patch" or "I will be +implementing this proposal by the end of next month." This often manifests as +reviews which may seem harsh or exacting; please keep in mind that the community +is trying to collaborate with you to build a patch that will benefit the project +on its own. + +## Code of Conduct + +We enthusiastically adhere to the same +[Code of Conduct](https://github.com/openbmc/docs/blob/master/code-of-conduct.md) +as the rest of OpenBMC. If you have any concerns, please check that document for +guidelines on who can help you resolve them. diff --git a/docs/testing.md b/docs/testing.md new file mode 100644 index 0000000..f5ed04b --- /dev/null +++ b/docs/testing.md @@ -0,0 +1,471 @@ +# 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 <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: + +- "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. |
