docs: Codify our preference for no return type in test functions (#1116)

* docs: Copy edit existing style guide

I can't help myself.

* docs: Codify our preference for no return type in test functions

docs/style_guide.md

@@ -1,33 +1,34 @@
 # Rationale and Goals
-As every Rust programmer knows, the language has many powerful features, and there are often several patterns which can express the same idea. Also, as every professional programmer comes to discover, code is almost always read far more than it is written.
 
-Thus, we choose to use a consistent set of idioms throughout our code so that it is easier to read and understand for both existing and new contributors.
+As every Rust programmer knows, the language has many powerful features, and there are often
+several patterns which can express the same idea. Also, as every professional programmer comes to
+discover, code is almost always read far more than it is written.
 
+Thus, we choose to use a consistent set of idioms throughout our code so that it is easier to read
+and understand for both existing and new contributors.
 
 ## Unsafe and Platform-Dependent conditional compilation
 
 ### Avoid `unsafe` Rust
 
-One of the main reasons to use Rust as an implementation language is
+One of the main reasons to use Rust as an implementation language is its strong memory safety
-its strong memory safety guarantees; Almost all of these guarantees
+guarantees; Almost all of these guarantees are voided by the use of `unsafe`. Thus, unless there is
-are voided by the use of `unsafe`. Thus, unless there is an excellent
+an excellent reason and the use is discussed beforehand, it is unlikely IOx will accept patches
-reason and the use is discussed beforehand, it is unlikely IOx will
+with `unsafe` code.
-accept patches with `unsafe` code.
 
 We may consider taking unsafe code given:
 
 1. performance benchmarks showing a *very* compelling improvement
-2. a compelling explanation of why the same performance can not be achieved using `safe` code
+1. a compelling explanation of why the same performance can not be achieved using `safe` code
-2. tests showing how it works safely across threads
+1. tests showing how it works safely across threads
 
-### Avoid platform specific conditional compilation `cfg`
+### Avoid platform-specific conditional compilation `cfg`
 
-We hope that IOx is usable across many different platforms and
+We hope that IOx is usable across many different platforms and Operating systems, which means we
-Operating systems, which means we put a high value on standard Rust.
+put a high value on standard Rust.
-
-While some performance critical code may require architecture specific
-instructions, (e.g. `AVX512`) most of the code should not.
 
+While some performance critical code may require architecture specific instructions, (e.g.
+`AVX512`) most of the code should not.
 
 ## Errors
 
@@ -48,36 +49,34 @@ pub enum Error {
 ```
 
 *Bad*:
+
 ```rust
 pub enum Error {
     NeedsAtLeastOneLine,
     // ...
 ```
 
-
-
 ### Use the `ensure!` macro to check a condition and return an error
 
 *Good*:
 
 * Reads more like an `assert!`
 * Is more concise
+
 ```rust
 ensure!(!self.schema_sample.is_empty(), NeedsAtLeastOneLine);
 ```
 
-*Bad*
+*Bad*:
+
 ```rust
 if self.schema_sample.is_empty() {
     return Err(Error::NeedsAtLeastOneLine {});
 }
 ```
 
-
 ### Errors should be defined in the module they are instantiated
 
-
-
 *Good*:
 
 * Groups related error conditions together most closely with the code that produces them
@@ -95,7 +94,8 @@ ensure!(foo.is_implemented(), NotImplemented {
 }
 ```
 
-*Bad*
+*Bad*:
+
 ```rust
 use crate::errors::NotImplemented;
 // ...
@@ -109,37 +109,38 @@ ensure!(foo.is_implemented(), NotImplemented {
 *Good*:
 
 * Reduces repetition
-```
+
+```rust
 pub type Result<T, E = Error> = std::result::Result<T, E>;
 ...
 fn foo() -> Result<bool> { true }
 ```
 
-*Bad*
+*Bad*:
-```
+
+```rust
 ...
 fn foo() -> Result<bool, Error> { true }
 ```
 
-
-
 ### `Err` variants should be returned with `fail()`
 
-*Good*
+*Good*:
+
 ```rust
 return NotImplemented {
     operation_name: "Parquet format conversion",
 }.fail();
 ```
 
-*Bad*
+*Bad*:
+
 ```rust
 return Err(Error::NotImplemented {
     operation_name: String::from("Parquet format conversion"),
 });
 ```
 
-
 ### Use `context` to wrap underlying errors into module specific errors
 
 *Good*:
@@ -154,7 +155,7 @@ input_reader
     })?;
 ```
 
-*Bad*
+*Bad*:
 
 ```rust
 input_reader
@@ -167,7 +168,8 @@ input_reader
 
 *Hint for `Box<dyn::std::error::Error>` in Snafu*:
 
-If your error contains a trait object (e.g. `Box<dyn std::error::Error + Send + Sync>`), in order to use  `context()` you need to wrap the error in a `Box` :
+If your error contains a trait object (e.g. `Box<dyn std::error::Error + Send + Sync>`), in order
+to use `context()` you need to wrap the error in a `Box`:
 
 ```rust
 #[derive(Debug, Snafu)]
@@ -187,12 +189,11 @@ database
   .await
   .map_err(|e| Box::new(e) as _)
   .context(ListingPartitions)?;
-
 ```
 
 Note the `as _` in the `map_err` call. Without it, you may get an error such as:
 
-```
+```console
 error[E0271]: type mismatch resolving `<ListingPartitions as IntoError<influxrpc::Error>>::Source == Box<<D as Database>::Error>`
   --> query/src/frontend/influxrpc.rs:63:14
    |
@@ -205,8 +206,7 @@ error[E0271]: type mismatch resolving `<ListingPartitions as IntoError<influxrpc
    = note: for more information, visit https://doc.rust-lang.org/book/ch19-03-advanced-traits.html
 ```
 
-
+### Each error cause in a module should have a distinct `Error` enum variant
-### Each error cause in a module should have a distinct Error enum
 
 Specific error types are preferred over a generic error with a `message` or `kind` field.
 
@@ -233,14 +233,13 @@ write_lines.context(UnableToWriteGoodLines)?;
 close_writer.context(UnableToCloseTableWriter))?;
 ```
 
-
+*Bad*:
-*Bad*
 
 ```rust
 pub enum Error {
     #[snafu(display("Error {}: {}", message, source))]
     WritingError {
-        source: IngestError ,
+        source: IngestError,
         message: String,
     },
 }
@@ -252,3 +251,48 @@ close_writer.context(WritingError {
     message: String::from("Error while closing the table writer"),
 })?;
 ```
+
+## Tests
+
+### Don't return `Result` from test functions
+
+At the time of this writing, if you return `Result` from test functions to use `?` in the test
+function body and an `Err` value is returned, the test failure message is not particularly helpful.
+Therefore, prefer not having a return type for test functions and instead using `expect` or
+`unwrap` in test function bodies.
+
+*Good*:
+
+```rust
+#[test]
+fn google_cloud() {
+    let config = Config::new();
+    let integration = ObjectStore::new_google_cloud_storage(GoogleCloudStorage::new(
+        config.service_account,
+        config.bucket,
+    ));
+
+    put_get_delete_list(&integration).unwrap();
+    list_with_delimiter(&integration).unwrap();
+}
+```
+
+*Bad*:
+
+```rust
+type TestError = Box<dyn std::error::Error + Send + Sync + 'static>;
+type Result<T, E = TestError> = std::result::Result<T, E>;
+
+#[test]
+fn google_cloud() -> Result<()> {
+    let config = Config::new();
+    let integration = ObjectStore::new_google_cloud_storage(GoogleCloudStorage::new(
+        config.service_account,
+        config.bucket,
+    ));
+
+    put_get_delete_list(&integration)?;
+    list_with_delimiter(&integration)?;
+    Ok(())
+}
+```