Google is committed to advancing racial equity for Black communities. See how.

Fuchsia Networking Contributor Guide

Fuchsia Networking welcomes contributions from all. This document defines contribution guidelines where they differ from or refine on the guidelines that apply to Fuchsia as a whole.

Getting Started

Consult the getting started document to set up your development environment.

Contributor Workflow

Consult the contribute changes document for general contribution guidance and project-wide best practices. The remainder of this document describes best practices specific to Fuchsia Networking.

Coding Guidelines

Philosophy

This section is inspired by Flutter's style guide, which contains many general principles that you should apply to all your programming work. Read it. The below calls out specific aspects that we feel are particularly important.

Be Lazy

Do not implement features you don't need. It is hard to correctly design unused code. This is closely related to the commit sizing advice given above; adding a new data structure to be used in some future commit is akin to adding a feature you don't need - it is exceedingly hard for your code reviewer to determine if you've designed the structure correctly because they (and you!) can't see how it is to be used.

Go Down the Rabbit Hole

You will occasionally encounter behaviour that surprises you or seems wrong. It probably is! Invest the time to find the root cause - you will either learn something, or fix something, and both are worth your time. Do not work around behaviour you don't understand.

Avoid Duplication

Avoid duplicating code whenever possible. In cases where existing code is not exposed in a manner suitable to your needs, prefer to extract the necessary parts into a common dependency.

Error Handling

Avoid unhandled errors and APIs which inherently disallow proper error handling; for a common example, consider fuchsia_async::executor::spawn. spawn inherently precludes error passing (since the flow of execution is severed). In most cases spawn can be replaced with a future that is later included in a select expression (example commit) or simply awaited on directly (example commit).

Avoid Implicit Drops

Do not implicitly discard values. In cases where a return value is unused, always be explicit. In Go, assign unused values to the blank identifier. In Rust, prefer destructuring assignment whenever possible, including when the return value is unit (()). When discarding primitive types where destructuring is not possible, use type annotations:

let _useless_rand: u8 = rand::thread_rng().gen();

When assigning structs, prefer explicit drops to implicitly discarding members using the rest pattern:

// Bad:
let FooStruct { foo_field_1, .. } = foo_instance;

// Good:
let FooStruct { foo_field_1, foo_field_2: _ } = foo_instance;

This way, when struct fields are changed, added, or removed, explicit acknowledgement will be required at the site of use.

Apply the same rules to unused closure parameters.

Compile-time over Run-time

Prefer type safety over runtime invariant checking. In other words, arrange your abstractions such that they cannot express invalid conditions rather than relying on assertions at runtime.

Write testable code; testable code is modular and its dependencies are easily injected.

Avoid magic numbers.

Comments

When writing comments, take a moment to consider the future reader of your comment. Ensure that your comments are complete sentences with proper grammar and punctuation. Note that adding more comments or more verbose comments is not always better; for example, avoid comments that repeat the code they're anchored on.

Documentation comments should be self-contained; in other words, do not assume that the reader is aware of documentation in adjacent files or on adjacent structures. Avoid documentation comments on types which describe instances of the type; for example, AddressSet is a set of client addresses. is a comment that describes a field of type AddressSet, but the type may be used to hold any kind of Address, not just a client's.

Phrase your comments to avoid references that might become stale; for example: do not mention a variable or type by name when possible (certain doc comments are necessary exceptions). Also avoid references to past or future versions of or past or future work surrounding the item being documented; explain things from first principles rather than making external references (including past revisions).

When writing TODOs:

  1. Include an issue reference using the format TODO(https://fxbug.dev/1245):
  2. Phrase the text as an action that is to be taken; it should be possible for another contributor to pick up the TODO without consulting any external sources, including the referenced issue.

Provide citations

When an implementation is following some specification/document (e.g. RFCs), include a comment with both a quote and citation of the relevant portion(s) of the document near the implementation. The quote lets readers know why something is being done and the citation allows a reader to get more context.

Error Messages

As with code comments, consider the future reader of the error messages emitted by your code. Ensure that your error messages are actionable. For example, avoid test failure messages such as "unexpected value" - always include the unexpected value; another example is "expected <variable> to be empty, was non-empty" - this message would be much more useful if it included the unexpected elements.

Always consider: what will the reader do with this message?

Tests

Consult the testability rubrics for general guidelines on test-writing and testability reviews on Fuchsia. In Fuchsia Networking, we define the following test classes:

  • Unit tests are fully local to a piece of code and all their external dependencies are faked or mocked.
  • Integration tests validate behavior between two or more different components.
  • End-to-end tests are driven by an external host machine and use the public APIs and bytes written to the network to perform behavior validation. Can be performed over a physical network or by virtualization of the DUT (qemu).

Consider the following guidelines when writing tests:

  1. Always add tests for new features or bug fixes.
  2. Consider the guidelines in Error Messages when writing test assertions.
  3. Tests must be deterministic. Threaded or time-dependent code, Random Number Generators (RNGs), and cross-component communication are common sources of nondeterminism.
    • Don't use sleep in tests as a means of weak synchronization. Only sleep when strictly necessary (e.g. when polling is required).
    • Time-dependent tests can use fake or mocked clocks to provide determinism. See fuchsia_async::Executor::new_with_fake_time and fake-clock.
    • Threaded code must always use the proper synchronization primitives to avoid flakes. Whenever possible, prefer single-threaded tests.
    • Always provide a mechanism to inject seeds for RNGs and use them in tests.
    • Test for flakes locally whenever possible; use repeat flags in test binaries (--test.count in Go, --gtest_repeat for googletest) and aim for at least 100-1000 runs locally if your test is prone to flakes before merging. > Rust test binaries currently don't have an equivalent flag, you may need to resort to a bash loop or equivalent to get repeated runs. See #65218.
  4. Avoid tests with hard-coded timeouts. Prefer relying on the framework/fixture to time out tests.
  5. Prefer hermetic tests; test set-up routines should be explicit and deterministic. Be mindful of test fixtures that run cases in parallel (such as Rust's) when using "ambient" services. Prefer to explicitly inject component dependencies that are vital to the test.
  6. Tests should always be components.
  7. Prefer virtual devices and networks for non-end-to-end tests. See netemul for guidance on virtual network environments.
  8. Avoid change detector tests; tests that are unnecessarily sensitive to changes, especially ones external to the code under test, can hamper feature development and refactoring.
  9. Do not encode implementation details in tests, prefer testing through a module's public API.
  10. Do not use Rust's support for tests which return Result; such tests do not automatically emit backtraces, relying on the errors themselves to carry a backtrace. Test failures that don't emit backtraces are typically much harder to interpret. At the time of writing, the backtrace feature in Rust is unstable and disabled in the Fuchsia Rust build configuration, but even if enabled this feature would only cause anyhow::Errors to carry backtraces; best to panic (via Result::expect) to avoid relying on external factors for backtraces.
  11. When unwrapping a Result<_, fidl::Error> returned from a FIDL method call, restate the function being called in the panic message to make it easier to track down the callsite. Don't repeat the type of the error, which is already included in the panic output. For example:

    // Bad:
    let foo_result = proxy
        .foo() // `foo` returns a `Result<_, fidl::Error>`.
        .await
        .expect("FIDL error"); // Doesn't provide any new information.
    
    // Good:
    let foo_result = proxy
        .foo() // `foo` returns a `Result<_, fidl::Error>`.
        .await
        .expect("calling foo"); // Restate the function being called.
    

Source Control Best Practices

Commits should be arranged for ease of reading; that is, incidental changes such as code movement or formatting changes should be committed separately from actual code changes.

Commits should always be focused. For example, a commit could add a feature, fix a bug, or refactor code, but not a mixture.

Commits should be thoughtfully sized; avoid overly large or complex commits which can be logically separated, but also avoid overly separated commits that require code reviews to load multiple commits into their mental working memory in order to properly understand how the various pieces fit together. If your changes require multiple commits, consider whether those changes warrant a design doc or RFC.

Commit Messages

Commit messages should be concise but self-contained (avoid relying on issue references as explanations for changes) and written such that they are helpful to people reading in the future (include rationale and any necessary context).

Avoid superfluous details or narrative.

Commit messages should consist of a brief subject line and a separate explanatory paragraph in accordance with the following:

  1. Separate subject from body with a blank line
  2. Limit the subject line to 50 characters
  3. Capitalize the subject line
  4. Do not end the subject line with a period
  5. Use the imperative mood in the subject line
  6. Wrap the body at 72 characters
  7. Use the body to explain what and why vs. how

The body may be omitted if the subject is self-explanatory; e.g. when fixing a typo. The git book contains a Commit Guidelines section with much of the same advice, and the list above is part of a blog post by Chris Beams.

Commit messages should make use of issue tracker integration. See Commit-log message integration in the monorail documentation.

When using issue tracker integration, don't omit necessary context that may also be included in the relevant issue (see "Commit messages should be concise but self-contained" above). Many issues are Google-internal, and any given issue tracker is not guaranteed to be usable at the time that the commit history is read.

Commit messages should never contain references to any of:

  1. Relative moments in time
  2. Non-public URLs
  3. Individuals
  4. Hosted code reviews (such as on fuchsia-review.googlesource.com)
    • Refer to commits in this repository by their SHA-1 hash
    • Refer to commits in other repositories by public web address (such as https://fuchsia.googlesource.com/fuchsia/+/67fec6d)
  5. Other entities which may not make sense to arbitrary future readers

Adding a Test: line to the commit message is encouraged. A Test: line should:

  1. Justify that any behavior changes or additions are thoroughly tested.
  2. Describe how to run new/affected test cases.

For example: Test: Added new unit tests. `fx test netstack-gotests`.

Code Review Guidelines

Code Review Flow

The following code review guidelines are adopted within the Netstack team:

  1. When your CL is ready for review, first request review from any team member not part of the leads group.
  2. Put leads in CC on the CL so they can follow any ongoing discussion and build context.
  3. You may prefer teammates that are more familiar with that area of the code, but that's not mandatory. One of the goals is to spread out.
  4. Once the initial reviewer approves your CL, the CC'ed lead commits to assigning themselves as a reviewer and sending out comments and/or approving within two working hours.

Be mindful that his scheme can increase latency for reviews, which is a side effect we'd like to minimize. Try to decrease your latency when asked for review and when addressing comments as well. Gerrit notification settings and smart e-mail filters can be a big help to drive those interrupts. Also, don't be afraid to ping for reviews.

Tips & Tricks

fx set

Run the following command to build all tests and their dependencies:

fx set core.x64 --with //src/connectivity/network:tests

If you're working on changes that affect fdio and third_party/go, add:

--with //sdk/lib/fdio:tests --with //third_party/go:go_stdlib_tests