From b27c7f4068bccd6593e7d9197e6dc1c9b2ffc837 Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Mon, 28 Feb 2022 08:46:39 +0000 Subject: [PATCH] docs: Add unit testing presentation Add the Kata Containers unit testing presentation I gave to the Kata outreach students as this may be of some use to others. Fixes: #3781 Signed-off-by: James O. D. Hunt --- docs/README.md | 5 + docs/presentations/README.md | 3 + docs/presentations/unit-testing/README.md | 14 + .../kata-containers-unit-testing.md | 335 ++++++++++++++++++ 4 files changed, 357 insertions(+) create mode 100644 docs/presentations/README.md create mode 100644 docs/presentations/unit-testing/README.md create mode 100644 docs/presentations/unit-testing/kata-containers-unit-testing.md diff --git a/docs/README.md b/docs/README.md index 973a00b839..529d772d97 100644 --- a/docs/README.md +++ b/docs/README.md @@ -58,6 +58,7 @@ Documents that help to understand and contribute to Kata Containers. ## Help Writing Unit Tests * [Unit Test Advice](Unit-Test-Advice.md) +* [Unit testing presentation](presentations/unit-testing/kata-containers-unit-testing.md) ## Help Improving the Documents @@ -72,6 +73,10 @@ Documents that help to understand and contribute to Kata Containers. * [Release strategy](Stable-Branch-Strategy.md) * [Release Process](Release-Process.md) +## Presentations + +* [Presentations](presentations) + ## Website Changes If you have a suggestion for how we can improve the diff --git a/docs/presentations/README.md b/docs/presentations/README.md new file mode 100644 index 0000000000..6219c2b5f5 --- /dev/null +++ b/docs/presentations/README.md @@ -0,0 +1,3 @@ +# Kata Containers presentations + +* [Unit testing](unit-testing) diff --git a/docs/presentations/unit-testing/README.md b/docs/presentations/unit-testing/README.md new file mode 100644 index 0000000000..658d89dc9f --- /dev/null +++ b/docs/presentations/unit-testing/README.md @@ -0,0 +1,14 @@ +# Kata Containers unit testing presentation + +## Markdown version + +See [the Kata Containers unit testing presentation](kata-containers-unit-testing.md). + +### To view as an HTML presentation + +```bash +$ infile="kata-containers-unit-testing.md" +$ outfile="/tmp/kata-containers-unit-testing.html" +$ pandoc -s --metadata title="Kata Containers unit testing" -f markdown -t revealjs --highlight-style="zenburn" -i -o "$outfile" "$infile" +$ xdg-open "file://$outfile" +``` diff --git a/docs/presentations/unit-testing/kata-containers-unit-testing.md b/docs/presentations/unit-testing/kata-containers-unit-testing.md new file mode 100644 index 0000000000..52ea699a1a --- /dev/null +++ b/docs/presentations/unit-testing/kata-containers-unit-testing.md @@ -0,0 +1,335 @@ +## Why write unit tests? + +- Catch regressions + +- Improve the code being tested + + Structure, quality, security, performance, "shakes out" implicit + assumptions, _etc_ + +- Extremely instructive + + Once you've fully tested a single function, you'll understand that + code very well indeed. + +## Why write unit tests? (continued) + +- Fun! + + Yes, really! Don't believe me? Try it! ;) + +## Run all Kata Containers agent unit tests + +As an example, to run all agent unit tests: + +```bash +$ cd $GOPATH/src/github.com/kata-containers/kata-containers +$ cd src/agent +$ make test +``` + +## List all unit tests + +- Identify the full name of all the tests _in the current package_: + + ```bash + $ cargo test -- --list + ``` + +- Identify the full name of all tests in the `foo` "local crate" + (sub-directory containing another `Cargo.toml` file): + + ```bash + $ cargo test -p "foo" -- --list + ``` + +## Run a single unit test + +- Run a test in the current package in verbose mode: + + ```bash + # Example + $ test="config::tests::test_get_log_level" + + $ cargo test "$test" -vv -- --exact --nocapture + ``` + +## Test coverage setup + +```bash +$ cargo install cargo-tarpaulin +``` + +## Show test coverage + +```bash +$ cd $GOPATH/src/github.com/kata-containers/kata-containers/src/agent +$ cargo -v tarpaulin --all-features --run-types AllTargets --count --force-clean -o Html +$ xdg-open "file://$PWD/tarpaulin-report.html" +``` + +## Testability (part 1) + +- To be testable, a function should: + - Not be "too long" (say >100 lines). + - Not be "too complex" (say >3 levels of indentation). + - Should return a `Result` or an `Option` so error paths + can be tested. + +- If functions don't conform, they need to be reworked (refactored) + before writing tests. + +## Testability (part 2) + +- Some functions can't be fully tested. +- However, you _can_ test the initial code that checks + the parameter values (test error paths only). + +## Writing new tests: General advice (part 1) + +- KISS: Keep It Simple Stupid + + You don't get extra points for cryptic code. + +- DRY: Don't Repeat Yourself + + Make use of existing facilities (don't "re-invert the wheel"). + +- Read the [unit test advice document](https://github.com/kata-containers/kata-containers/blob/main/docs/Unit-Test-Advice.md) + +## Writing new tests: General advice (part 2) + +- Attack the function in all possible ways + +- Use the _table driven_ approach: + - Simple + - Compact + - Easy to debug + - Makes boundary analysis easy + - Encourages functions to be testable + +## Writing new tests: Specific advice (part 1) + +- Create a new "`tests`" module if necessary. +- Give each test function a "`test_`" prefix. +- Add the "`#[test]`" annotation on each test function. + +## Writing new tests: Specific advice (part 2) + +- If you need to `use` (import) packages for the tests, + _only do it in the `tests` module_: + ```rust + use some_test_pkg::{foo, bar}; // <-- Not here + + #[cfg(test)] + mod tests { + use super::*; + use some_test_pkg:{foo, bar}; // <-- Put it here + } + ``` + +## Writing new tests: Specific advice (part 3) + +- You can add test-specific dependencies in `Cargo.toml`: + ```toml + [dev-dependencies] + serial_test = "0.5.1" + ``` + +## Writing new tests: Specific advice (part 4) + +- Don't add in lots of error handling code: let the test panic! + ```rust + // This will panic if the unwrap fails. + // - NOT acceptable generally for production code. + // - PERFECTLY acceptable for test code since: + // - Keeps the test code simple. + // - Rust will detect the panic and fail the test. + let result = func().unwrap(); + ``` + +## Debugging tests (part 1) + +- Comment out all tests in your `TestData` array apart from the failing test. + +- Add temporary `println!("FIXME: ...")` statements in the code. + +- Set `RUST_BACKTRACE=full` before running `cargo test`. + +## Debugging tests (part 2) + +- Use a debugger (not normally necessary though): + ```bash + # Disable optimisation + $ RUSTFLAGS="-C opt-level=0" cargo test --no-run + + # Find the test binary + $ test_binary=$(find target/debug/deps | grep "kata_agent-[a-z0-9][a-z0-9]*$" | tail -1) + + $ rust-gdb "$test_binary" + ``` + +## Useful tips + +- Always start a test with a "clean environment": + + Create new set of objects / files / directories / _etc_ + for each test. + +- Mounts + - Linux allows mounts on top of existing mounts. + - Bind mounts and read-only mounts can be useful. + +## Gotchas (part 1) + +If a test runs successfully _most of the time_: + +- Review the test logic. + +- Add a `#[serial]` annotation on the test function + Requires the `serial_test` package in the `[dev-dependencies]` + section of `Cargo.toml`. + + If this makes it work the test is probably sharing resources with + another task (thread). + +## Gotchas (part 2) + +If a test works locally but fails in the CI, consider the following +attributes of each environment (local and CI): + +- The version of rust being used. +- The hardware architecture. +- Number (and spec) of the CPUs. + +## Gotchas (part 3) + +If in doubt, look at the +["test artifacts" attached to the failing CI test](http://jenkins.katacontainers.io). + +## Before raising a PR + +- Remember to check that the test runs locally: + - As a non-privileged user. + - As the `root` user (carefully!) + +- Run the [static checker](https://github.com/kata-containers/tests/blob/main/.ci/static-checks.sh) + on your changes. + + Checks formatting and many other things. + +## If in doubt + +- Ask for help! ;) + +## Quiz 1 + +What's wrong with this function? + +```rust +fn foo(config: &Config, path_prefix: String, container_id: String, pid: String) -> Result<()> { + let mut full_path = format!("{}/{}", path_prefix, container_id); + + let _ = remove_recursively(&mut full_path); + + write_number_to_file(pid, full_path); + + Ok(()) +} +``` + +## Quiz 1: Answers (part 1) + +- No check that `path_prefix`, `container_id` and `pid` are not `""`. +- No check that `path_prefix` is absolute. +- No check that `container_id` does not contain slashes / contains only valid characters. +- Result of `remove_recursively()` discarded. +- `remove_recursively()` _may_ modify `full_path` without `foo()` knowing! + +## Quiz 1: Answers (part 2) + +- Why is `pid` not a numeric? +- No check to ensure the PID is positive. +- No check to recreate any directories in the original `path_prefix`. +- `write_number_to_file()` could fail so why doesn't it return a value? +- The `config` parameter is unused. + +## Quiz 1: What if... + +Imagine if the caller managed to do this: + +```rust +foo(config, "", "sbin/init", r#"#!/bin/sh\n/sbin/reboot"#); +``` + +## Quiz 2 + +What makes this function difficult to test? + +```rust +fn get_user_id(username: String) -> i32 { + let line = grep_file(username, "/etc/passwd").unwrap(); + let fields = line.split(':'); + + let uid = fields.nth(2).ok_or("failed").unwrap(); + + uid.parse::() +} +``` + +## Quiz 2: Answers (part 1) + +- Unhelpful error message ("failed"). + +- Panics on error! Return a `Result` instead! + +- UID's cannot be negative so function should return an unsigned + value. + +## Quiz 2: Answers (part 2) + +- Hard-coded filename. + + This would be better: + + ```rust + const PASSWD_DB: &str = "/etc/passwd"; + + // Test code can now pass valid and invalid files! + fn get_user_id(filename: String, username: String) -> i32 { + // ... + } + + let id = get_user_id(PASSWD_DB, username); + ``` + +## Quiz 3 + +What's wrong with this test code? + +```rust +let mut obj = Object::new(); + +// Sanity check +assert_eq!(obj.num, 0); +assert_eq!(obj.wibble, false); + +// Test 1 +obj->foo_method(7); +assert_eq!(obj.num, 7); + +// Test 2 +obj->bar_method(true); +assert_eq!(obj.wibble, true); +``` + +## Quiz 3: Answers + +- The test code is "fragile": + - The 2nd test re-uses the object created in the first test. + +## Finally + +- [We need a GH action to run the unit tests](https://github.com/kata-containers/kata-containers/issues/2934) + + Needs to fail PRs that decrease test coverage
by "x%".