From bc9558149ca8e4ba6279a1b7ec2e9a7ef8387b7b Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Mon, 22 Nov 2021 16:28:07 +0000 Subject: [PATCH 01/12] docs: Move doc requirements section higher Move the documentation requirements document link up so that it appears immediately below the "How to Contribute" section. Signed-off-by: James O. D. Hunt --- docs/README.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/README.md b/docs/README.md index 5dedec3786..fcb76cf344 100644 --- a/docs/README.md +++ b/docs/README.md @@ -52,6 +52,10 @@ 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 Improving the Documents + +* [Documentation Requirements](Documentation-Requirements.md) + ### Code Licensing * [Licensing](Licensing-strategy.md): About the licensing strategy of Kata Containers. @@ -61,10 +65,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 From cf360fad9224ac0a8d2d4709a8c973be0ab169f2 Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Tue, 23 Nov 2021 09:45:01 +0000 Subject: [PATCH 02/12] docs: Move unit test advice doc from tests repo Unit tests necessarily need to be maintained with the code they test so it makes sense to keep the Unit Test Advice document into the main repo since that is where the majority of unit tests reside. Note: The [`Unit-Test-Advice.md` file](https://github.com/kata-containers/tests/blob/main/Unit-Test-Advice.md) was copied from the `tests` repo when it's `HEAD` was https://github.com/kata-containers/tests/commit/38855f1f40120f4f009fa30061e9affc07930640. Signed-off-by: James O. D. Hunt --- docs/README.md | 4 + docs/Unit-Test-Advice.md | 333 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 337 insertions(+) create mode 100644 docs/Unit-Test-Advice.md diff --git a/docs/README.md b/docs/README.md index fcb76cf344..d1efc14ba5 100644 --- a/docs/README.md +++ b/docs/README.md @@ -52,6 +52,10 @@ 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 Unit Tests + +* [Unit Test Advice](Unit-Test-Advice.md) + ## Help Improving the Documents * [Documentation Requirements](Documentation-Requirements.md) diff --git a/docs/Unit-Test-Advice.md b/docs/Unit-Test-Advice.md new file mode 100644 index 0000000000..88663c9760 --- /dev/null +++ b/docs/Unit-Test-Advice.md @@ -0,0 +1,333 @@ +# Unit Test Advice + +* [Overview](#overview) +* [Assertions](#assertions) + * [golang assertions](#golang-assertions) + * [rust assertions](#rust-assertions) +* [Table driven tests](#table-driven-tests) + * [golang table driven tests](#golang-table-driven-tests) + * [rust table driven tests](#rust-table-driven-tests) +* [Temporary files](#temporary-files) + * [golang temporary files](#golang-temporary-files) + * [rust temporary files](#rust-temporary-files) +* [User running the test](#user-running-the-test) + * [running golang tests as different users](#running-golang-tests-as-different-users) + * [running rust tests as different users](#running-rust-tests-as-different-users) + +## Overview + +This document offers advice on writing a Unit Test (UT) in +[`golang`](https://golang.org) and [`rust`](https://www.rust-lang.org). + +## 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. +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 == "" { + 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 ... + } +} + +``` + +## User running the test + +[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. + +Some repositories already provide utility functions to skip a test: + +- if running as `root` +- if not running as `root` + +### running golang tests as different users + +The runtime repository has the most comprehensive set of skip abilities. See: + +- https://github.com/kata-containers/kata-containers/tree/main/src/runtime/pkg/katatestutils + +### running rust tests as different users + +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 + + // ... + } +} +``` From 597b239ef3ef78feb689eb60df1e71130efedd6e Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Tue, 23 Nov 2021 09:54:35 +0000 Subject: [PATCH 03/12] docs: Remove TOC in UT advice doc Remove the table of contents in the Unit Test Advice document since GitHub auto-generates these now. See: https://github.com/kata-containers/kata-containers/pull/2023 Signed-off-by: James O. D. Hunt --- docs/Unit-Test-Advice.md | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/docs/Unit-Test-Advice.md b/docs/Unit-Test-Advice.md index 88663c9760..8bdbfd4c66 100644 --- a/docs/Unit-Test-Advice.md +++ b/docs/Unit-Test-Advice.md @@ -1,19 +1,5 @@ # Unit Test Advice -* [Overview](#overview) -* [Assertions](#assertions) - * [golang assertions](#golang-assertions) - * [rust assertions](#rust-assertions) -* [Table driven tests](#table-driven-tests) - * [golang table driven tests](#golang-table-driven-tests) - * [rust table driven tests](#rust-table-driven-tests) -* [Temporary files](#temporary-files) - * [golang temporary files](#golang-temporary-files) - * [rust temporary files](#rust-temporary-files) -* [User running the test](#user-running-the-test) - * [running golang tests as different users](#running-golang-tests-as-different-users) - * [running rust tests as different users](#running-rust-tests-as-different-users) - ## Overview This document offers advice on writing a Unit Test (UT) in From c1111a1d2d7720cbf0087b384eeb29d5c3fee31c Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Tue, 23 Nov 2021 09:56:27 +0000 Subject: [PATCH 04/12] docs: Use leading caps for lang names in UT advice doc Use a capital letter when referring to Golang and Rust (and remove unnecessary backticks for Rust). > **Note:** > > We continue refer to "Go" as "Golang" since it's a common alias, > but, crucially, familiarity with this name makes searching for > information using this term possible: "Go" is too generic a word. Signed-off-by: James O. D. Hunt --- docs/Unit-Test-Advice.md | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/docs/Unit-Test-Advice.md b/docs/Unit-Test-Advice.md index 8bdbfd4c66..f4231dff7a 100644 --- a/docs/Unit-Test-Advice.md +++ b/docs/Unit-Test-Advice.md @@ -3,11 +3,11 @@ ## Overview This document offers advice on writing a Unit Test (UT) in -[`golang`](https://golang.org) and [`rust`](https://www.rust-lang.org). +[Golang](https://golang.org) and [Rust](https://www.rust-lang.org). ## Assertions -### golang 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: @@ -21,7 +21,7 @@ func TestSomething(t *testing.T) { } ``` -### rust assertions +### Rust assertions Use the standard set of `assert!()` macros. @@ -32,7 +32,7 @@ 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 +### Golang table driven tests Assume the following function: @@ -113,7 +113,7 @@ func TestJoinParamsWithDash(t *testing.T) { } ``` -### rust table driven tests +### Rust table driven tests Assume the following function: @@ -216,7 +216,7 @@ mod tests { Always delete temporary files on success. -### golang temporary files +### Golang temporary files ```go func TestSomething(t *testing.T) { @@ -233,7 +233,7 @@ func TestSomething(t *testing.T) { } ``` -### rust temporary files +### Rust temporary files Use the `tempfile` crate which allows files and directories to be deleted automatically: @@ -273,13 +273,13 @@ Some repositories already provide utility functions to skip a test: - if running as `root` - if not running as `root` -### running golang tests as different users +### running Golang tests as a different user The runtime repository has the most comprehensive set of skip abilities. See: - https://github.com/kata-containers/kata-containers/tree/main/src/runtime/pkg/katatestutils -### running rust tests as different users +### running Rust tests as a different user One method is to use the `nix` crate along with some custom macros: From e8bb6b26660e745bd625fe14bc89634e2b930c8d Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Tue, 23 Nov 2021 10:20:39 +0000 Subject: [PATCH 05/12] docs: Correct repo name usage Change reference from "runtime repo" to "main repo" in unit test advice document. Signed-off-by: James O. D. Hunt --- docs/Unit-Test-Advice.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/Unit-Test-Advice.md b/docs/Unit-Test-Advice.md index f4231dff7a..8d3b7b94ed 100644 --- a/docs/Unit-Test-Advice.md +++ b/docs/Unit-Test-Advice.md @@ -275,7 +275,7 @@ Some repositories already provide utility functions to skip a test: ### running Golang tests as a different user -The runtime repository has the most comprehensive set of skip abilities. See: +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 From 318b3f187bef9f073c3fe73de72f65c0818e31ff Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Tue, 23 Nov 2021 10:21:29 +0000 Subject: [PATCH 06/12] docs: No present continuous in UT advice doc Change some headings to avoid using the present continuous tense which should not be used for headings. Signed-off-by: James O. D. Hunt --- docs/Unit-Test-Advice.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/Unit-Test-Advice.md b/docs/Unit-Test-Advice.md index 8d3b7b94ed..e88d8acf0f 100644 --- a/docs/Unit-Test-Advice.md +++ b/docs/Unit-Test-Advice.md @@ -257,7 +257,7 @@ mod tests { ``` -## User running the test +## Test user [Unit tests are run *twice*](https://github.com/kata-containers/tests/blob/main/.ci/go-test.sh): @@ -273,13 +273,13 @@ Some repositories already provide utility functions to skip a test: - if running as `root` - if not running as `root` -### running Golang tests as a different user +### 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 -### running Rust tests as a different user +### Run Rust tests as a different user One method is to use the `nix` crate along with some custom macros: From 9fed7d0bde7dbcce1f1128a9f5e1e6e52b4c0d52 Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Tue, 23 Nov 2021 10:25:16 +0000 Subject: [PATCH 07/12] docs: Mention anyhow for error handling in UT doc Add a comment stating that `anyhow` and `thiserror` should be used in real rust code, rather than the unwieldy default `Result` handling shown in the example. Signed-off-by: James O. D. Hunt --- docs/Unit-Test-Advice.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/Unit-Test-Advice.md b/docs/Unit-Test-Advice.md index e88d8acf0f..07af3922f1 100644 --- a/docs/Unit-Test-Advice.md +++ b/docs/Unit-Test-Advice.md @@ -120,6 +120,8 @@ 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. From fcf45b0c92d152ddfb222de32eaf8f9a0fe1f321 Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Tue, 23 Nov 2021 10:26:47 +0000 Subject: [PATCH 08/12] docs: Use more idiomatic rust string check Rather than comparing a string to a literal in the rust example, use `.is_empty()` as that approach is more idiomatic and preferred. Signed-off-by: James O. D. Hunt --- docs/Unit-Test-Advice.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/Unit-Test-Advice.md b/docs/Unit-Test-Advice.md index 07af3922f1..cc46486a08 100644 --- a/docs/Unit-Test-Advice.md +++ b/docs/Unit-Test-Advice.md @@ -129,7 +129,7 @@ pub type Result = std::result::Result; // 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 == "" { + if str.is_empty() { return Err("string cannot be blank"); } From baf4f76d97da9c57f6a6fb0a22ed4cb69f47dbbb Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Tue, 23 Nov 2021 10:28:47 +0000 Subject: [PATCH 09/12] docs: More detail on running tests as different users Add some more detail to the unit test advice document about running tests as different users. Signed-off-by: James O. D. Hunt --- docs/Unit-Test-Advice.md | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/docs/Unit-Test-Advice.md b/docs/Unit-Test-Advice.md index cc46486a08..b877f8eb86 100644 --- a/docs/Unit-Test-Advice.md +++ b/docs/Unit-Test-Advice.md @@ -268,12 +268,9 @@ mod tests { 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. - -Some repositories already provide utility functions to skip a test: - -- if running as `root` -- if not running as `root` +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 From d41c375c4f2a8a97016fa56ee9e27cf0b61b47f3 Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Tue, 23 Nov 2021 10:29:21 +0000 Subject: [PATCH 10/12] docs: Add more advice to the UT advice doc Add information to the unit test advice document on test strategies and the test environment. Signed-off-by: James O. D. Hunt --- docs/Unit-Test-Advice.md | 61 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/docs/Unit-Test-Advice.md b/docs/Unit-Test-Advice.md index b877f8eb86..52dce452bd 100644 --- a/docs/Unit-Test-Advice.md +++ b/docs/Unit-Test-Advice.md @@ -5,6 +5,67 @@ 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 From aff32756081b472be0c7a5f1283c323e2d926df2 Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Tue, 23 Nov 2021 10:49:33 +0000 Subject: [PATCH 11/12] docs: Add a code PR advice document Add a document giving advice to code PR authors. Fixes: #3099. Signed-off-by: James O. D. Hunt --- docs/README.md | 4 + docs/code-pr-advice.md | 246 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 250 insertions(+) create mode 100644 docs/code-pr-advice.md diff --git a/docs/README.md b/docs/README.md index d1efc14ba5..f5fd38eef7 100644 --- a/docs/README.md +++ b/docs/README.md @@ -52,6 +52,10 @@ 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) 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). From f0734f52c1325ad064cc9256fbbed4a4d4466778 Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Thu, 25 Nov 2021 14:44:07 +0000 Subject: [PATCH 12/12] docs: Remove extraneous whitespace Remove trailing whitespace in the unit test advice doc. Signed-off-by: James O. D. Hunt --- docs/Unit-Test-Advice.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/Unit-Test-Advice.md b/docs/Unit-Test-Advice.md index 52dce452bd..5fe8b9cafc 100644 --- a/docs/Unit-Test-Advice.md +++ b/docs/Unit-Test-Advice.md @@ -222,7 +222,7 @@ mod tests { num: i32, result: Result, } - + // The tests can now be specified as a set of inputs and outputs let tests = &[ // Failure scenarios @@ -249,24 +249,24 @@ mod tests { 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);