diff --git a/docs/README.md b/docs/README.md index 5dedec3786..f5fd38eef7 100644 --- a/docs/README.md +++ b/docs/README.md @@ -52,6 +52,18 @@ Documents that help to understand and contribute to Kata Containers. * [How to contribute to Kata Containers](https://github.com/kata-containers/community/blob/master/CONTRIBUTING.md) * [Code of Conduct](../CODE_OF_CONDUCT.md) +## Help Writing a Code PR + +* [Code PR advice](code-pr-advice.md). + +## Help Writing Unit Tests + +* [Unit Test Advice](Unit-Test-Advice.md) + +## Help Improving the Documents + +* [Documentation Requirements](Documentation-Requirements.md) + ### Code Licensing * [Licensing](Licensing-strategy.md): About the licensing strategy of Kata Containers. @@ -61,10 +73,6 @@ Documents that help to understand and contribute to Kata Containers. * [Release strategy](Stable-Branch-Strategy.md) * [Release Process](Release-Process.md) -## Help Improving the Documents - -* [Documentation Requirements](Documentation-Requirements.md) - ## Website Changes If you have a suggestion for how we can improve the diff --git a/docs/Unit-Test-Advice.md b/docs/Unit-Test-Advice.md new file mode 100644 index 0000000000..5fe8b9cafc --- /dev/null +++ b/docs/Unit-Test-Advice.md @@ -0,0 +1,379 @@ +# Unit Test Advice + +## Overview + +This document offers advice on writing a Unit Test (UT) in +[Golang](https://golang.org) and [Rust](https://www.rust-lang.org). + +## General advice + +### Unit test strategies + +#### Positive and negative tests + +Always add positive tests (where success is expected) *and* negative +tests (where failure is expected). + +#### Boundary condition tests + +Try to add unit tests that exercise boundary conditions such as: + +- Missing values (`null` or `None`). +- Empty strings and huge strings. +- Empty (or uninitialised) complex data structures + (such as lists, vectors and hash tables). +- Common numeric values (such as `-1`, `0`, `1` and the minimum and + maximum values). + +#### Test unusual values + +Also always consider "unusual" input values such as: + +- String values containing spaces, Unicode characters, special + characters, escaped characters or null bytes. + + > **Note:** Consider these unusual values in prefix, infix and + > suffix position. + +- String values that cannot be converted into numeric values or which + contain invalid structured data (such as invalid JSON). + +#### Other types of tests + +If the code requires other forms of testing (such as stress testing, +fuzz testing and integration testing), raise a GitHub issue and +reference it on the issue you are using for the main work. This +ensures the test team are aware that a new test is required. + +### Test environment + +#### Create unique files and directories + +Ensure your tests do not write to a fixed file or directory. This can +cause problems when running multiple tests simultaneously and also +when running tests after a previous test run failure. + +#### Assume parallel testing + +Always assume your tests will be run *in parallel*. If this is +problematic for a test, force it to run in isolation using the +`serial_test` crate for Rust code for example. + +### Running + +Ensure you run the unit tests and they all pass before raising a PR. +Ideally do this on different distributions on different architectures +to maximise coverage (and so minimise surprises when your code runs in +the CI). + +## Assertions + +### Golang assertions + +Use the `testify` assertions package to create a new assertion object as this +keeps the test code free from distracting `if` tests: + +```go +func TestSomething(t *testing.T) { + assert := assert.New(t) + + err := doSomething() + assert.NoError(err) +} +``` + +### Rust assertions + +Use the standard set of `assert!()` macros. + +## Table driven tests + +Try to write tests using a table-based approach. This allows you to distill +the logic into a compact table (rather than spreading the tests across +multiple test functions). It also makes it easy to cover all the +interesting boundary conditions: + +### Golang table driven tests + +Assume the following function: + +```go +// The function under test. +// +// Accepts a string and an integer and returns the +// result of sticking them together separated by a dash as a string. +func joinParamsWithDash(str string, num int) (string, error) { + if str == "" { + return "", errors.New("string cannot be blank") + } + + if num <= 0 { + return "", errors.New("number must be positive") + } + + return fmt.Sprintf("%s-%d", str, num), nil +} +``` + +A table driven approach to testing it: + +```go +import ( + "testing" + "github.com/stretchr/testify/assert" +) + +func TestJoinParamsWithDash(t *testing.T) { + assert := assert.New(t) + + // Type used to hold function parameters and expected results. + type testData struct { + param1 string + param2 int + expectedResult string + expectError bool + } + + // List of tests to run including the expected results + data := []testData{ + // Failure scenarios + {"", -1, "", true}, + {"", 0, "", true}, + {"", 1, "", true}, + {"foo", 0, "", true}, + {"foo", -1, "", true}, + + // Success scenarios + {"foo", 1, "foo-1", false}, + {"bar", 42, "bar-42", false}, + } + + // Run the tests + for i, d := range data { + // Create a test-specific string that is added to each assert + // call. It will be displayed if any assert test fails. + msg := fmt.Sprintf("test[%d]: %+v", i, d) + + // Call the function under test + result, err := joinParamsWithDash(d.param1, d.param2) + + // update the message for more information on failure + msg = fmt.Sprintf("%s, result: %q, err: %v", msg, result, err) + + if d.expectError { + assert.Error(err, msg) + + // If an error is expected, there is no point + // performing additional checks. + continue + } + + assert.NoError(err, msg) + assert.Equal(d.expectedResult, result, msg) + } +} +``` + +### Rust table driven tests + +Assume the following function: + +```rust +// Convenience type to allow Result return types to only specify the type +// for the true case; failures are specified as static strings. +// XXX: This is an example. In real code use the "anyhow" and +// XXX: "thiserror" crates. +pub type Result = std::result::Result; + +// The function under test. +// +// Accepts a string and an integer and returns the +// result of sticking them together separated by a dash as a string. +fn join_params_with_dash(str: &str, num: i32) -> Result { + if str.is_empty() { + return Err("string cannot be blank"); + } + + if num <= 0 { + return Err("number must be positive"); + } + + let result = format!("{}-{}", str, num); + + Ok(result) +} + +``` + +A table driven approach to testing it: + +```rust +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_join_params_with_dash() { + // This is a type used to record all details of the inputs + // and outputs of the function under test. + #[derive(Debug)] + struct TestData<'a> { + str: &'a str, + num: i32, + result: Result, + } + + // The tests can now be specified as a set of inputs and outputs + let tests = &[ + // Failure scenarios + TestData { + str: "", + num: 0, + result: Err("string cannot be blank"), + }, + TestData { + str: "foo", + num: -1, + result: Err("number must be positive"), + }, + + // Success scenarios + TestData { + str: "foo", + num: 42, + result: Ok("foo-42".to_string()), + }, + TestData { + str: "-", + num: 1, + result: Ok("--1".to_string()), + }, + ]; + + // Run the tests + for (i, d) in tests.iter().enumerate() { + // Create a string containing details of the test + let msg = format!("test[{}]: {:?}", i, d); + + // Call the function under test + let result = join_params_with_dash(d.str, d.num); + + // Update the test details string with the results of the call + let msg = format!("{}, result: {:?}", msg, result); + + // Perform the checks + if d.result.is_ok() { + assert!(result == d.result, msg); + continue; + } + + let expected_error = format!("{}", d.result.as_ref().unwrap_err()); + let actual_error = format!("{}", result.unwrap_err()); + assert!(actual_error == expected_error, msg); + } + } +} +``` + +## Temporary files + +Always delete temporary files on success. + +### Golang temporary files + +```go +func TestSomething(t *testing.T) { + assert := assert.New(t) + + // Create a temporary directory + tmpdir, err := ioutil.TempDir("", "") + assert.NoError(err) + + // Delete it at the end of the test + defer os.RemoveAll(tmpdir) + + // Add test logic that will use the tmpdir here... +} +``` + +### Rust temporary files + +Use the `tempfile` crate which allows files and directories to be deleted +automatically: + +```rust +#[cfg(test)] +mod tests { + use tempfile::tempdir; + + #[test] + fn test_something() { + + // Create a temporary directory (which will be deleted automatically + let dir = tempdir().expect("failed to create tmpdir"); + + let filename = dir.path().join("file.txt"); + + // create filename ... + } +} + +``` + +## Test user + +[Unit tests are run *twice*](https://github.com/kata-containers/tests/blob/main/.ci/go-test.sh): + +- as the current user +- as the `root` user (if different to the current user) + +When writing a test consider which user should run it; even if the code the +test is exercising runs as `root`, it may be necessary to *only* run the test +as a non-`root` for the test to be meaningful. Add appropriate skip +guards around code that requires `root` and non-`root` so that the test +will run if the correct type of user is detected and skipped if not. + +### Run Golang tests as a different user + +The main repository has the most comprehensive set of skip abilities. See: + +- https://github.com/kata-containers/kata-containers/tree/main/src/runtime/pkg/katatestutils + +### Run Rust tests as a different user + +One method is to use the `nix` crate along with some custom macros: + +``` +#[cfg(test)] +mod tests { + #[allow(unused_macros)] + macro_rules! skip_if_root { + () => { + if nix::unistd::Uid::effective().is_root() { + println!("INFO: skipping {} which needs non-root", module_path!()); + return; + } + }; + } + + #[allow(unused_macros)] + macro_rules! skip_if_not_root { + () => { + if !nix::unistd::Uid::effective().is_root() { + println!("INFO: skipping {} which needs root", module_path!()); + return; + } + }; + } + + #[test] + fn test_that_must_be_run_as_root() { + // Not running as the superuser, so skip. + skip_if_not_root!(); + + // Run test *iff* the user running the test is root + + // ... + } +} +``` diff --git a/docs/code-pr-advice.md b/docs/code-pr-advice.md new file mode 100644 index 0000000000..78413ccf23 --- /dev/null +++ b/docs/code-pr-advice.md @@ -0,0 +1,246 @@ +# Code PR Advice + +Before raising a PR containing code changes, we suggest you consider +the following to ensure a smooth and fast process. + +> **Note:** +> +> - All the advice in this document is optional. However, if the +> advice provided is not followed, there is no guarantee your PR +> will be merged. +> +> - All the check tools will be run automatically on your PR by the CI. +> However, if you run them locally first, there is a much better +> chance of a successful initial CI run. + +## Assumptions + +This document assumes you have already read (and in the case of the +code of conduct agreed to): + +- The [Kata Containers code of conduct](https://github.com/kata-containers/community/blob/main/CODE_OF_CONDUCT.md). +- The [Kata Containers contributing guide](https://github.com/kata-containers/community/blob/main/CONTRIBUTING.md). + +## Code + +### Architectures + +Do not write architecture-specific code if it is possible to write the +code generically. + +### General advice + +- Do not write code to impress: instead write code that is easy to read and understand. + +- Always consider which user will run the code. Try to minimise + the privileges the code requires. + +### Comments + +Always add comments if the intent of the code is not obvious. However, +try to avoid comments if the code could be made clearer (for example +by using more meaningful variable names). + +### Constants + +Don't embed magic numbers and strings in functions, particularly if +they are used repeatedly. + +Create constants at the top of the file instead. + +### Copyright and license + +Ensure all new files contain a copyright statement and an SPDX license +identifier in the comments at the top of the file. + +### FIXME and TODO + +If the code contains areas that are not fully implemented, make this +clear a comment which provides a link to a GitHub issue that provides +further information. + +Do not just rely on comments in this case though: if possible, return +a "`BUG: feature X not implemented see {bug-url}`" type error. + +### Functions + +- Keep functions relatively short (less than 100 lines is a good "rule of thumb"). + +- Document functions if the parameters, return value or general intent + of the function is not obvious. + +- Always return errors where possible. + + Do not discard error return values from the functions this function + calls. + +### Logging + +- Don't use multiple log calls when a single log call could be used. + +- Use structured logging where possible to allow + [standard tooling](https://github.com/kata-containers/tests/tree/main/cmd/log-parser) + be able to extract the log fields. + +### Names + +Give functions, macros and variables clear and meaningful names. + +### Structures + +#### Golang structures + +Unlike Rust, Go does not enforce that all structure members be set. +This has lead to numerous bugs in the past where code like the +following is used: + +```go +type Foo struct { + Key string + Value string +} + +// BUG: Key not set, but nobody noticed! ;( +let foo1 = Foo { + Value: "foo", +} +``` + +A much safer approach is to create a constructor function to enforce +integrity: + +```go +type Foo struct { + Key string + Value string +} + +func NewFoo(key, value string) (*Foo, error) { + if key == "" { + return nil, errors.New("Foo needs a key") + } + + if value == "" { + return nil, errors.New("Foo needs a value") + } + + return &Foo{ + Key: key, + Value: value, + }, nil +} + +func testFoo() error { + // BUG: Key not set, but nobody noticed! ;( + badFoo := Foo{Value: "value"} + + // Ok - the constructor performs needed validation + goodFoo, err := NewFoo("name", "value") + if err != nil { + return err + } + + return nil +``` + +> **Note:** +> +> The above is just an example. The *safest* approach would be to move +> `NewFoo()` into a separate package and make `Foo` and it's elements +> private. The compiler would then enforce the use of the constructor +> to guarantee correctly defined objects. + + +### Tracing + +Consider if the code needs to create a new +[trace span](https://github.com/kata-containers/kata-containers/blob/main/docs/tracing.md). + +Ensure any new trace spans added to the code are completed. + +## Tests + +### Unit tests + +Where possible, code changes should be accompanied by unit tests. + +Consider using the standard +[table-based approach](https://github.com/kata-containers/tests/blob/main/Unit-Test-Advice.md) +as it encourages you to make functions small and simple, and also +allows you to think about what types of value to test. + +### Other categories of test + +Raised a GitHub issue in the +[`tests`](https://github.com/kata-containers/tests) repository that +explains what sort of test is required along with as much detail as +possible. Ensure the original issue is referenced on the `tests` issue. + +### Unsafe code + +#### Rust language specifics + +Minimise the use of `unsafe` blocks in Rust code and since it is +potentially dangerous always write [unit tests][#unit-tests] +for this code where possible. + +`expect()` and `unwrap()` will cause the code to panic on error. +Prefer to return a `Result` on error rather than using these calls to +allow the caller to deal with the error condition. + +The table below lists the small number of cases where use of +`expect()` and `unwrap()` are permitted: + +| Area | Rationale for permitting | +|-|-| +| In test code (the `tests` module) | Panics will cause the test to fail, which is desirable. | +| `lazy_static!()` | This magic macro cannot "return" a value as it runs before `main()`. | +| `defer!()` | Similar to golang's `defer()` but doesn't allow the use of `?`. | +| `tokio::spawn(async move {})` | Cannot currently return a `Result` from an `async move` closure. | +| If an explicit test is performed before the `unwrap()` / `expect()` | *"Just about acceptable"*, but not ideal `[*]` | + + +`[*]` - There can lead to bad *future* code: consider what would +happen if the explicit test gets dropped in the future. This is easier +to happen if the test and the extraction of the value are two separate +operations. In summary, this strategy can introduce an insidious +maintenance issue. + +## Documentation + +### General requirements + +- All new features should be accompanied by documentation explaining: + + - What the new feature does + + - Why it is useful + + - How to use the feature + + - Any known issues or limitations + + Links should be provided to GitHub issues tracking the issues + +- The [documentation requirements document](Documentation-Requirements.md) + explains how the project formats documentation. + +### Markdown syntax + +Run the +[markdown checker](https://github.com/kata-containers/tests/tree/main/cmd/check-markdown) +on your documentation changes. + +### Spell check + +Run the +[spell checker](https://github.com/kata-containers/tests/tree/main/cmd/check-spelling) +on your documentation changes. + +## Finally + +You may wish to read the documentation that the +[Kata Review Team](https://github.com/kata-containers/community/blob/main/Rota-Process.md) use to help review PRs: + +- [PR review guide](https://github.com/kata-containers/community/blob/main/PR-Review-Guide.md). +- [documentation review process](https://github.com/kata-containers/community/blob/main/Documentation-Review-Process.md).