WIP Add explicit documentation for our testing strategy.

This also shuffles a few bits of testing infrastructure around to
try to make things clearer and/or more consistent.

.circleci/config.yml

@@ -141,26 +141,8 @@ commands:
       # Test with 1. only default features on, 2. all features on, 3. no features on.
       # This is not perfect (really we want the cartesian product), but is good enough in practice.
       - run:
-          name: Test
-          command: cargo test --all --verbose
-      - run:
-          name: Test (all features)
-          command: cargo test --all --all-features --verbose
-      - run:
-          name: Test (no default features)
-          # Apparently --no-default-features in the root has never worked. So
-          # just pull the list of packages out of cargo metadata and run the
-          # test command for each package separately.
-          command: |
-            for package in $(cargo metadata --format-version 1 | jq -r '.workspace_members[] | sub(" .*$"; "")'); do
-              # For some reason cargo even rejects `-p package` for this, so we
-              # have to pull out the manifest... There's probably a way to make
-              # jq do this as part of the prev expression if you know how to use
-              # it, but whatever.
-              manifest=$(cargo metadata --format-version 1 | jq -r '.packages[] | select(.name == $pkg) | .manifest_path' --arg pkg $package)
-              echo "## no-default-features test for $package (manifest @ $manifest)"
-              cargo test --manifest-path $manifest --no-default-features --verbose
-            done
+          name: Test (default features, no features, and all features)
+          command: bash automation/all_rust_tests.sh --verbose
   dependency-checks:
     steps:
       - run:
@@ -256,18 +238,7 @@ jobs:
       - test-setup
       - run: rustup component add clippy
       - run: cargo clippy --version
-      - run: cargo clippy --all --all-targets --all-features -- -D warnings
-      # --no-default-features does not work on a workspace, so we do it for every crate.
-      - run: |
-          for package in $(cargo metadata --format-version 1 | jq -r '.workspace_members[] | sub(" .*$"; "")'); do
-            # For some reason cargo even rejects `-p package` for this, so we
-            # have to pull out the manifest... There's probably a way to make
-            # jq do this as part of the prev expression if you know how to use
-            # it, but whatever.
-            manifest=$(cargo metadata --format-version 1 | jq -r '.packages[] | select(.name == $pkg) | .manifest_path' --arg pkg $package)
-            echo "## no-default-features clippy for $package (manifest @ $manifest)"
-            cargo clippy --manifest-path $manifest --all-targets --no-default-features -- -D warnings
-          done
+      - run: bash automation/all_clippy_checks.sh
   Lint Bash scripts:
     docker:
       - image: koalaman/shellcheck-alpine:stable
@@ -366,7 +337,7 @@ jobs:
             xcrun instruments -w "iPhone 8 (13.0) [" || true
       - run:
           name: Run XCode tests
-          command: bash run-ios-tests.sh
+          command: bash automation/run_ios_tests.sh
       - store_artifacts:
           path: raw_xcodetest.log
           destination: logs/raw_xcodetest.log

.github/pull_request_template.md

@@ -1,11 +1,7 @@
 ### Pull Request checklist ###
 <!-- Before submitting the PR, please address each item -->
 - [ ] **Quality**: This PR builds and tests run cleanly
-  - `cargo test --all` produces no test failures
-  - `cargo clippy --all --all-targets --all-features` runs without emitting any warnings
-  - `cargo fmt` does not produce any changes to the code
-  - `./gradlew ktlint detekt` runs without emitting any warnings
-  - `swiftformat --swiftversion 4 megazords components/*/ios && swiftlint` runs without emitting any warnings or producing changes
+  - `automation/all_tests.sh` runs to completion and produces no failures
   - Note: For changes that need extra cross-platform testing, consider adding `[ci full]` to the PR title.
 - [ ] **Tests**: This PR includes thorough tests or an explanation of why it does not
 - [ ] **Changelog**: This PR includes a changelog entry in [CHANGES_UNRELEASED.md](../CHANGES_UNRELEASED.md) or an explanation of why it does not need one

automation/all_clippy_checks.sh

@@ -0,0 +1,31 @@
+#!/usr/bin/env bash
+#
+# A convenience wrapper for running clippy across all packages.
+# This is complicated by rust's feature system, so we want to run:
+#
+#  1. Clippy with all features enabled
+#  2. Clippy with no features enabled
+#
+
+set -eux
+
+if [[ ! -f "$PWD/automation/all_clippy_checks.sh" ]]
+then
+    echo "all_clippy_checks.sh must be executed from the root directory."
+    exit 1
+fi
+
+EXTRA_ARGS=( "$@" )
+
+cargo clippy --all --all-targets --all-features -- -D warnings  "${EXTRA_ARGS[@]}"
+
+# Apparently --no-default-features doesn't work in the root, even with -p to select a specific package.
+# Instead we pull the list of individual package manifest files out of `cargo metadata` and
+# test using --manifest-path for each individual package.
+# This is a really gross way to parse JSON, but works with no external depdenencies...
+for manifest in $(cargo metadata --format-version 1 --no-deps | tr -s '"' '\n' | grep 'Cargo.toml'); do
+    package=$(dirname "$manifest")
+    package=$(basename "$package")
+    echo "## no-default-features clippy for package $package (manifest @ $manifest)"
+    cargo clippy --manifest-path "$manifest" --all-targets --no-default-features -- -D warnings "${EXTRA_ARGS[@]}"
+done

automation/all_rust_tests.sh

@@ -0,0 +1,35 @@
+#!/usr/bin/env bash
+#
+# A convenience wrapper for running the full suite of rust tests.
+# This is complicated by rust's feature system, so we want to run:
+#
+#  1. Tests with only the default features on
+#  2. Tests with all features on
+#  3. Tests with no features on
+#
+# This is not perfect (really we want the cartesian product), but is good enough in practice.
+
+set -eux
+
+if [[ ! -f "$PWD/automation/all_rust_tests.sh" ]]
+then
+    echo "all_rust_tests.sh must be executed from the root directory."
+    exit 1
+fi
+
+EXTRA_ARGS=( "$@" )
+
+#cargo test --all "${EXTRA_ARGS[@]}"
+
+#cargo test --all --all-features "${EXTRA_ARGS[@]}"
+
+# Apparently --no-default-features doesn't work in the root, even with -p to select a specific package.
+# Instead we pull the list of individual package manifest files out of `cargo metadata` and
+# test using --manifest-path for each individual package.
+# This is a really gross way to parse JSON, but works with no external depdenencies...
+for manifest in $(cargo metadata --format-version 1 --no-deps | tr -s '"' '\n' | grep 'Cargo.toml'); do
+    package=$(dirname "$manifest")
+    package=$(basename "$package")
+    echo "## no-default-features test for package $package (manifest @ $manifest)"
+    cargo test --manifest-path "$manifest" --no-default-features "${EXTRA_ARGS[@]}"
+done

automation/all_tests.sh

@@ -0,0 +1,57 @@
+#!/usr/bin/env bash
+#
+# A convenience wrapper for running the full suite of tests and checks
+# required before submitting a PR.
+#
+# There are surprisingly many things to run to fully check all the code,
+# since we are simultaneously a rust project, a kotlin/gradle project,
+# and a switch project!
+
+set -eux
+
+if [[ ! -f "$PWD/automation/all_tests.sh" ]]
+then
+    echo "all_tests.sh must be executed from the root directory."
+    exit 1
+fi
+
+# Linters. These should all pass before merging, and running them
+# first may help us fail quicker if something is wrong..
+
+./automation/all_clippy_checks.sh
+
+./gradlew ktlint detekt
+
+if [[ "$(uname -s)" == "Darwin" ]]
+then
+    swiftlint --strict
+else
+   echo "WARNING: skipping swiftlint on non-Darwin host"
+fi
+
+# Test suites. These should all pass before merging.
+
+./automation/all_rust_tests.sh
+
+cargo run -p sync-test
+
+./gradlew test
+
+if [[ "$(uname -s)" == "Darwin" ]]
+then
+    ./automation/run_ios_tests.sh
+else
+    echo "WARNING: skipping iOS tests on non-Darwin host"
+fi
+
+# Formatters. These should always succeed, but might leave
+# uncomitted changes in your working directory.
+
+cargo fmt
+
+if [[ "$(uname -s)" == "Darwin" ]]
+then
+    swiftformat megazords components/*/ios --lint --swiftversion 4 
+else
+    echo "WARNING: skipping swiftformat on non-Darwin host"
+fi

docs/README.md

@@ -25,6 +25,7 @@ you work on app-services projects.  We have:
   * Howtos for specific coding activities:
     * Code and architecture guidelines:
       * [Guide to Building a Rust Component](./howtos/building-a-rust-component.md)
+      * [Guide to Testing a Rust Component](./howtos/testing-a-rust-component.md)
       * [How to expose your Rust Component to Kotlin](./howtos/exposing-rust-components-to-kotlin.md)
       * [How to expose your Rust Component to Swift](./howtos/exposing-rust-components-to-swift.md)
       * [How to pass data cross the FFI boundary](./howtos/when-to-use-what-in-the-ffi.md)

docs/contributing.md

@@ -24,16 +24,15 @@ Build instructions are available [here](building.md). Do not hesitate to let us
 Patches should be submitted as [pull requests](https://help.github.com/articles/about-pull-requests/) (PRs).
 
 Before submitting a PR:
-- Your code must run and pass all the automated tests before you submit your PR for review. "Work in progress" pull requests are allowed to be submitted, but should be clearly labeled as such and should not be merged until all tests pass and the code has been reviewed.
-  - Run `cargo test --all` to make sure all tests still pass and no warnings are emitted.
-  - Run `cargo run -p sync-test` to make sure the sync integration tests still pass.
-  Note: You need to have [node](https://nodejs.org/) installed for running the integration tests.
-  - Run `cargo clippy --all --all-targets --all-features` to make sure that linting passes (You may need to `rustup component add clippy` first).
-  - Run `cargo fmt` to ensure the code is formatted correctly.
-  - If you have modified any Kotlin code, check that `./gradlew ktlint detekt` runs without emitting any warnings.
-  - If you have modified any Swift code, check that `swiftformat --swiftversion 4 megazords components/*/ios && swiftlint` runs without emitting any warnings or producing changes.
+- Run `cargo fmt` to ensure your Rust code is correctly formatted.
+  - If you have modified any Swift code, also run `swiftformat --swiftversion 4` on the modified code.
+- Your code must run and pass all the automated tests before you submit your PR for review.
+  - The simplest way to confirm this is to use the `./automation/all_tests.sh` script, which runs all test suites
+    and linters for Rust, Kotlin and Swift code.
+  - "Work in progress" pull requests are welcome, but should be clearly labeled as such and should not be merged until all tests pass and the code has been reviewed.
 - Your patch should include new tests that cover your changes, or be accompanied by explanation for why it doesn't need any. It is your
   and your reviewer's responsibility to ensure your patch includes adequate tests.
+  - Consult the [testing guide](./howtos/testing-a-rust-component.md) for some tips on writing effective tests.
 - Your patch should include a changelog entry in [CHANGES_UNRELEASED.md](../CHANGES_UNRELEASED.md) or an explanation of why
   it does not need one. Any breaking changes to Swift or Kotlin binding APIs should be noted explicitly
 - If your patch adds new dependencies, they must follow our [dependency management guidelines](./dependency-management.md).

docs/howtos/testing-a-rust-component.md

@@ -0,0 +1,144 @@
+
+# Guide to Testing a Rust Component
+
+This document gives a high-level overview of how we test components in application-services.
+It will be useful to you if you're adding a new component, or working on increasing the test
+coverage of an existing component.
+
+If you are only interested in running the existing test suite, please consult the
+[contributor docs](../contributing.md) and the [all_tests.sh](../../automation/all_tests.sh) script.
+
+## Unit and Functional Tests
+
+### Rust code
+
+Since the core implementation of our components lives in rust, so does the core of our testing strategy.
+
+Each rust component should be accompanied by a suite of unittests, following the [guidelines for writing
+tests](https://doc.rust-lang.org/book/ch11-00-testing.html) from the [Rust
+Book](https://doc.rust-lang.org/book/title-page.html).
+Some additional tips:
+
+* Where possible, it's better use use the Rust typesystem to make bugs impossible than to write
+  tests to assert that they don't occur in practice. But given that the ultimate consumers of our
+  code are not in Rust, that's sometimes not possible. The best idiomatic Rust API for a feature
+  is not necessarily the best API for consuming it over an FFI boundary!
+
+* Rust's builtin assertion macros are pretty spartan; we use the [more_asserts](https://crates.io/crates/more_asserts)
+  for some additional helpers.
+
+* Rust's strict typing can make test mocks difficult. If there's something you need to mock out in tests,
+  make it a Trait and use the [mockiato](https://crates.io/crates/mockiato) crate to mock it out.
+
+The Rust tests for a component should be runnable via `cargo test`.
+
+### FFI Layer code
+
+We currently do not test the FFI-layer Rust code for our components, since it's generally a very thin
+wrapper around the underlying (and in theory well-tested!) Rust component code. If you find yourself
+adding a particularly complex bit of code in an FFI-layer crate, add unittests in the same style as
+for other Rust code.
+
+(Editor's note: I remain hopeful that one day we'll autogenerate most of the FFI-layer code, and in
+such a world we don't need to invest in tests for it.)
+
+### Kotlin code
+
+The Kotlin wrapper code for a component should have its own test suite, which should follow the general guidelines for
+[testing Android code in Mozilla projects](https://github.com/mozilla-mobile/shared-docs/blob/master/android/testing.md#jvm-testing).
+In practice that means we use
+[JUnit](https://github.com/mozilla-mobile/shared-docs/blob/master/android/testing.md#junit-testing-framework)
+as the test framework and
+[Robolectric](https://github.com/mozilla-mobile/shared-docs/blob/master/android/testing.md#robolectric-android-api-shadows)
+to provide implementations of Android-specific APIs.
+
+The Kotlin tests for a component should be runnable via `./gradlew <component>:test`.
+
+The tests at this layer are designed to ensure that the API binding code is working as intended,
+and should not repeat tests for functionality that is already well tested at the Rust level.
+But given that the Kotlin bindings involve a non-trivial amount of hand-written boilerplate code,
+it's important to exercise that code throughly.
+
+One complication with running Kotlin tests is that the code needs to run on your local development machine,
+but the Kotlin code's native dependencies are typically compiled and packaged for Android devices. The
+tests need to ensure that an appropriate version of JNA and of the compiled Rust code is available in
+their library search path at runtime. Our `build.gradle` files contain a collection of hackery that ensures
+this, which should be copied into any new components.
+
+(Editor's note: I remain hopeful that one day we'll autogenerate most of the Kotlin binding code, and in
+such a world we don't need to invest in tests for it.)
+
+XXX TODO: talk about proguard? I don't really understand it...
+
+XXX TODO: any additional tips here, such as mocking out storage etc?
+
+### Swift code
+
+The Swift wrapper code for a component should have its own test suite, using Apple's
+[XCode unittest framework](https://developer.apple.com/documentation/xctest).
+
+Due to the way that all rust components need to be compiled together into a single ["megazord"](../design/megazords.md)
+framework, this entire respository is a single XCode project. The Swift tests for each component
+thus need to live under `megazords/ios/MozillaAppServicesTests/` rather than in the directory
+for the corresponding component. (XXX TODO: is this true? it would be nice to find a way to avoid havining
+them live separately because it makes them easy to overlook).
+
+The tests at this layer are designed to ensure that the API binding code is working as intended,
+and should not repeat tests for functionality that is already well tested at the Rust level.
+But given that the Swift bindings involve a non-trivial amount of hand-written boilerplate code,
+it's important to exercise that code throughly.
+
+(Editor's note: I remain hopeful that one day we'll autogenerate most of the Swift binding code, and in
+such a world we don't need to invest in tests for it.)
+
+XXX TODO: any additional tips here, such as mocking out storage etc?
+
+## Integration tests
+
+### End-to-end Sync Tests
+
+The [`testing/sync-test`](../../testing/sync-test) directory contains a test harness for running sync-related
+Rust components against a live Firefox Sync infrastructure, so that we can verifying the functionality
+end-to-end.
+
+Each component that implements a sync engine should have a corresponding suite of tests in this directory.
+
+* XXX TODO: places doesn't.
+* XXX TODO: send-tab doesn't (not technically a sync engine, but still, it's related)
+* XXX TODO: sync-manager doesn't
+
+### Android Components Test Suite
+
+It's important that changes in application-services are tested against upstream consumer code in the
+[android-components](https://github.com/mozilla-mobile/android-components/) repo. This is currently
+a manual process involving:
+
+* Configuring your local checkout of android-components to [use your local application-services
+  build](./working-with-reference-browser.md).
+* Running the android-components test suite via `./gradle test`.
+* Manually building and running the android-components sample apps to verify that they're still working.
+
+Ideally some or all of this would be automated and run in CI, but we have not yet invested in such automation.
+
+## Test Coverage
+
+Lamentably, we do not measure or report on code test coverage.
+See [this github issue](https://github.com/mozilla/application-services/issues/1745) for some early explorations.
+
+The rust ecosystem for code coverage is still maturing, with [cargo-tarpaulin](https://github.com/xd009642/tarpaulin)
+appearing to be a promising candidate.  However, such tools will only report code that is exercised by the rust
+unittests, not code that is exercised by the Kotlin or Swift tests or the end-to-end integration test suite.
+
+For code coverage to be useful to us, we need to either:
+
+* Commit to ensuring high coverage via rust-level tests alone, or
+* Figure out how to measure it for code being driven by non-rust test suites.
+
+## Ideas for Improvement
+
+* ASan, Memsan, and maybe other sanitizer checks, especially around the points where we cross FFI boundaries.
+* General-purpose fuzzing, such as via https://github.com/jakubadamw/arbitrary-model-tests
+* We could consider making a mocking backend for viaduct, which would also be mockable from Kotlin/Swift.
+* Add more end-to-end integration tests!
+* Live device tests, e.g. actual Fenixes running in an emulator and syncing to each other.
+* Run consumer integration tests in CI against master.

testing/sync-test/README.md

@@ -0,0 +1,27 @@
+# End-to-End Tests for Sync
+
+This package implements "end-to-end" integration tests for syncing various data types -
+two clients using a real live account and a real live sync server, exchanging
+data and asserting that the exchange works as intended.
+
+## Running the tests
+
+Run the tests using `cargo run`.
+
+Use `cargo run -- --help` to see the available options.
+
+Running the tests currently requires nodejs, in order to drive a headless browser.
+There is an [open issue](https://github.com/mozilla/application-services/issues/2403)
+to investigate how to remove this dependency.
+
+## Adding tests
+
+For each datatype managed by sync, there should be a suite of corresponding tests.
+To add some:
+
+0. In `auth.rs`, add support your sync engine to the `TestClient` struct.
+0. Create a file `./src/<datatype>.rs` to hold the tests; `logins.rs` may provide a useful example.
+  0. Create a `test_<name>` function for each scenario you want to exercise. The function should take
+     two `TestClient` instances as arguments, and use them to drive a simulated sync between two clients.
+  0. Define a `get_test_group()` function that returns your test scenarios in a `TestGroup` struct.
+0. Add your test group to the `main` function defined in `main.rs` for execution.