Merge pull request #747 from Carrotman42/export_tmp

Internal Change.

go/best-practices.md

@@ -175,7 +175,7 @@ These examples mostly use stubs. Update your names accordingly if your code uses
 fakes or another kind of test double.
 
 [naming]: guide#naming
-[test doubles]: https://en.wikipedia.org/wiki/Test_double
+[test doubles]: https://abseil.io/resources/swe-book/html/ch13.html#basic_concepts
 
 Suppose you have a well-focused package providing production code similar to
 this:
@@ -465,7 +465,7 @@ func (s *Server) innerHandler(ctx context.Context, req *pb.MyRequest) *pb.MyResp
     // Unconditionally cap the deadline for this part of request handling.
     ctx, cancel := context.WithTimeout(ctx, 3*time.Second)
     defer cancel()
-    ctxlog.Info("Capped deadline in inner request")
+    ctxlog.Info(ctx, "Capped deadline in inner request")
 
     // Code here no longer has access to the original context.
     // This is good style if when first writing this, you anticipate
@@ -1434,7 +1434,7 @@ Documentation is strongly encouraged if:
     // Good:
     package health
 
-    // A Watcher reports the health of some entity (usually a backen service).
+    // A Watcher reports the health of some entity (usually a backend service).
     //
     // Watcher methods are safe for simultaneous use by multiple goroutines.
     type Watcher interface {
@@ -2252,7 +2252,7 @@ func FuzzFencepost(f *testing.F) {
     f.Fuzz(func(t *testing.T, geo Place, padding Length) {
         got := Fencepost(geo, padding)
         // Simple reference implementation: not used in prod, but easy to
-        // reasonable and therefore useful to check against in random tests.
+        // reason about and therefore useful to check against in random tests.
         reference := slowFencepost(geo, padding)
 
         // In the fuzz test, inputs and outputs can be large so don't
@@ -2457,9 +2457,11 @@ underlying transport to connect to the test version of the backend.
 For example, suppose the code you want to test (sometimes referred to as "system
 under test" or SUT) interacts with a backend that implements the
 [long running operations] API. To test your SUT, use a real [OperationsClient]
-that is connected to a [test double](https://en.wikipedia.org/wiki/Test_double)
+that is connected to a
+[test double](https://abseil.io/resources/swe-book/html/ch13.html#basic_concepts)
 (e.g., a mock, stub, or fake) of the [OperationsServer].
 
+[test double]: https://abseil.io/resources/swe-book/html/ch13.html#basic_concepts
 [long running operations]: https://pkg.go.dev/google.golang.org/genproto/googleapis/longrunning
 [OperationsClient]: https://pkg.go.dev/google.golang.org/genproto/googleapis/longrunning#OperationsClient
 [OperationsServer]: https://pkg.go.dev/google.golang.org/genproto/googleapis/longrunning#OperationsServer
@@ -2648,7 +2650,7 @@ they must not call these functions from inside these goroutines.
 
 [Test helpers](#test-functions) usually don't signal failure from new
 goroutines, and therefore it is all right for them to use `t.Fatal`. If in
-doubt, call t.Error and return instead.
+doubt, call `t.Error` and return instead.
 
 ```go
 // Good:
@@ -3052,3 +3054,356 @@ usage := "" +
 -->
 
 {% endraw %}
+
+<a id="globals"></a>
+
+## Global state
+
+Libraries should not force their clients to use APIs that rely on
+[global state](https://en.wikipedia.org/wiki/Global_variable). They are advised
+not to expose APIs or export
+[package level](https://go.dev/ref/spec#TopLevelDecl) variables that control
+behavior for all clients as parts of their API. The rest of the section uses
+"global" and "package level state" synonymously.
+
+Instead, if your functionality maintains state, allow your clients to create and
+use instance values.
+
+**Important:** While this guidance is applicable to all developers, it is most
+critical for infrastructure providers who offer libraries, integrations, and
+services to other teams.
+
+```go
+// Good:
+// Package sidecar manages subprocesses that provide features for applications.
+package sidecar
+
+type Registry struct { plugins map[string]*Plugin }
+
+func New() *Registry { return &Registry{plugins: make(map[string]*Plugin)} }
+
+func (r *Registry) Register(name string, p *Plugin) error { ... }
+```
+
+Your users will instantiate the data they need (a `*sidecar.Registry`) and then
+pass it as an explicit dependency:
+
+```go
+// Good:
+package main
+
+func main() {
+  sidecars := sidecar.New()
+  if err := sidecars.Register("Cloud Logger", cloudlogger.New()); err != nil {
+    log.Exitf("could not setup cloud logger: %v", err)
+  }
+  cfg := &myapp.Config{Sidecars: sidecars}
+  myapp.Run(context.Background(), cfg)
+}
+```
+
+There are different approaches to migrating existing code to support dependency
+passing. The main one you will use is passing dependencies as parameters to
+constructors, functions, methods, or struct fields on the call chain.
+
+See also:
+
+*   [Go Tip #5: Slimming Your Client Libraries](https://google.github.io/styleguide/go/index.html#gotip)
+*   [Go Tip #24: Use Case-Specific Constructions](https://google.github.io/styleguide/go/index.html#gotip)
+*   [Go Tip #40: Improving Time Testability with Function Parameters](https://google.github.io/styleguide/go/index.html#gotip)
+*   [Go Tip #41: Identify Function Call Parameters](https://google.github.io/styleguide/go/index.html#gotip)
+*   [Go Tip #44: Improving Time Testability with Struct Fields](https://google.github.io/styleguide/go/index.html#gotip)
+*   [Go Tip #80: Dependency Injection Principles](https://google.github.io/styleguide/go/index.html#gotip)
+
+APIs that do not support explicit dependency passing become fragile as the
+number of clients increases:
+
+```go
+// Bad:
+package sidecar
+
+var registry = make(map[string]*Plugin)
+
+func Register(name string, p *Plugin) error { /* registers plugin in registry */ }
+```
+
+Consider what happens in the case of tests exercising code that transitively
+relies on a sidecar for cloud logging.
+
+```go
+// Bad:
+package app
+
+import (
+  "cloudlogger"
+  "sidecar"
+  "testing"
+)
+
+func TestEndToEnd(t *testing.T) {
+  // The system under test (SUT) relies on a sidecar for a production cloud
+  // logger already being registered.
+  ... // Exercise SUT and check invariants.
+}
+
+func TestRegression_NetworkUnavailability(t *testing.T) {
+  // We had an outage because of a network partition that rendered the cloud
+  // logger inoperative, so we added a regression test to exercise the SUT with
+  // a test double that simulates network unavailability with the logger.
+  sidecar.Register("cloudlogger", cloudloggertest.UnavailableLogger)
+  ... // Exercise SUT and check invariants.
+}
+
+func TestRegression_InvalidUser(t *testing.T) {
+  // The system under test (SUT) relies on a sidecar for a production cloud
+  // logger already being registered.
+  //
+  // Oops. cloudloggertest.UnavailableLogger is still registered from the
+  // previous test.
+  ... // Exercise SUT and check invariants.
+}
+```
+
+Go tests are executed sequentially by default, so the tests above run as:
+
+1.  `TestEndToEnd`
+2.  `TestRegression_NetworkUnavailability`, which overrides the default value of
+    cloudlogger
+3.  `TestRegression_InvalidUser`, which requires the default value of
+    cloudlogger registered in `package sidecar`
+
+This creates an order-dependent test case, which breaks running with test
+filters, and prevents tests from running in parallel or being sharded.
+
+Using global state poses problems that lack easy answers for you and the API's
+clients:
+
+*   What happens if a client needs to use different and separately operating
+    sets of `Plugin`s (for example, to support multiple servers) in the same
+    process space?
+
+*   What happens if a client wants to replace a registered `Plugin` with an
+    alternative implementation in a test, like a [test double]?
+
+    What happens if a client's tests require hermeticity between instances of a
+    `Plugin`, or between all of the plugins registered?
+
+*   What happens if multiple clients `Register` a `Plugin` under the same name?
+    Which one wins, if any?
+
+    How should errors be [handled](decisions#handle-errors)? If the code panics
+    or calls `log.Fatal`, will that always be
+    [appropriate for all places in which API would be called](decisions#dont-panic)?
+    Can a client verify it doesn't do something bad before doing so?
+
+*   Are there certain stages in a program's startup phases or lifetime during
+    which `Register` can be called and when it can't?
+
+    What happens if `Register` is called at the wrong time? A client could call
+    `Register` in [`func init`](https://go.dev/ref/spec#Package_initialization),
+    before flags are parsed, or after `main`. The stage at which a function is
+    called affects error handling. If the author of an API assumes the API is
+    *only* called during program initialization without the requirement that it
+    is, the assumption may nudge the author to design error handling to
+    [abort the program](best-practices#program-init) by modeling the API as a
+    `Must`-like function. Aborting is not appropriate for general-purpose
+    library functions that can be used at any stage.
+
+*   What if the client's and the designer's concurrency needs are mismatched?
+
+See also:
+
+*   [Go Tip #36: Enclosing Package-Level State](https://google.github.io/styleguide/go/index.html#gotip)
+*   [Go Tip #71: Reducing Parallel Test Flakiness](https://google.github.io/styleguide/go/index.html#gotip)
+*   [Go Tip #80: Dependency Injection Principles](https://google.github.io/styleguide/go/index.html#gotip)
+*   Error Handling:
+    [Look Before You Leap](https://docs.python.org/3/glossary.html#term-LBYL)
+    versus
+    [Easier to Ask for Forgiveness than Permission](https://docs.python.org/3/glossary.html#term-EAFP)
+*   [Unit Testing Practices on Public APIs]
+
+Global state has cascading effects on the
+[health of the Google codebase](guide.md#maintainability). Global state should
+be approached with **extreme scrutiny**.
+
+[Global state comes in several forms](#globals-forms), and you can use a few
+[litmus tests to identify when it is safe](#globals-litmus-tests).
+
+[Unit Testing Practices on Public APIs]: index.md#unit-testing-practices
+
+<a id="globals-forms"></a>
+
+### Major forms of package state APIs
+
+Several of the most common problematic API forms are enumerated below:
+
+*   Top-level variables irrespective of whether they are exported.
+
+    ```go
+    // Bad:
+    package logger
+
+    // Sinks manages the default output sources for this package's logging API.  This
+    // variable should be set at package initialization time and never thereafter.
+    var Sinks []Sink
+    ```
+
+    See the [litmus tests](#globals-litmus-tests) to know when these are safe.
+
+*   The
+    [service locator pattern](https://en.wikipedia.org/wiki/Service_locator_pattern).
+    See the [first example](#globals). The service locator pattern itself is not
+    problematic, rather the locator being defined as global.
+
+*   Registries for
+    [callbacks](https://en.wikipedia.org/wiki/Callback_\(computer_programming\))
+    and similar behaviors.
+
+    ```go
+    // Bad:
+    package health
+
+    var unhealthyFuncs []func
+
+    func OnUnhealthy(f func()) {
+      unhealthyFuncs = append(unhealthyFuncs, f)
+    }
+    ```
+
+*   Thick-Client singletons for things like backends, storage, data access
+    layers, and other system resources. These often pose additional problems
+    with service reliability.
+
+    ```go
+    // Bad:
+    package useradmin
+
+    var client pb.UserAdminServiceClientInterface
+
+    func Client() *pb.UserAdminServiceClient {
+        if client == nil {
+            client = ...  // Set up client.
+        }
+        return client
+    }
+    ```
+
+> **Note:** Many legacy APIs in the Google codebase do not follow this guidance;
+> in fact, some Go standard libraries allow for configuration via global values.
+> Nevertheless, the legacy API's contravention of this guidance
+> **[should not be used as precedent](guide#local-consistency)** for continuing
+> the pattern.
+>
+> It is better to invest in proper API design today than pay for redesigning
+> later.
+
+<a id="globals-litmus-tests"></a>
+
+### Litmus tests
+
+[APIs using the patterns above](#globals-forms) are unsafe when:
+
+*   Multiple functions interact via global state when executed in the same
+    program, despite being otherwise independent (for example, authored by
+    different authors in vastly different directories).
+*   Independent test cases interact with each other through global state.
+*   Users of the API are tempted to swap or replace global state for testing
+    purposes, particularly to replace any part of the state with a
+    [test double], like a stub, fake, spy, or mock.
+*   Users have to consider special ordering requirements when interacting with
+    global state: `func init`, whether flags are parsed yet, etc.
+
+Provided the conditions above are avoided, there are a **few limited
+circumstances under which these APIs are safe**, namely when any of the
+following is true:
+
+*   The global state is logically constant
+    ([example](https://github.com/klauspost/compress/blob/290f4cfacb3eff892555a491e3eeb569a48665e7/zstd/snappy.go#L413)).
+*   The package's observable behavior is stateless. For example, a public
+    function may use a private global variable as a cache, but so long as the
+    caller can't distinguish cache hits from misses, the function is stateless.
+*   The global state does not bleed into things that are external to the
+    program, like sidecar processes or files on a shared filesystem.
+*   There is no expectation of predictable behavior
+    ([example](https://pkg.go.dev/math/rand)).
+
+> **Note:**
+> [Sidecar processes](https://www.oreilly.com/library/view/designing-distributed-systems/9781491983638/ch02.html)
+> may **not** strictly be process-local. They can and often are shared with more
+> than one application process. Moreover, these sidecars often interact with
+> external distributed systems.
+>
+> Further, the same stateless, idempotent, and local rules in addition to the
+> base considerations above would apply to the code of the sidecar process
+> itself!
+
+An example of one of these safe situations is
+[`package image`](https://pkg.go.dev/image) with its
+[`image.RegisterFormat`](https://pkg.go.dev/image#RegisterFormat) function.
+Consider the litmus tests from above applied to a typical decoder, like the one
+for handling the [PNG](https://pkg.go.dev/image/png) format:
+
+*   Multiple calls to `package image`'s APIs that use the registered decoders
+    (for example, `image.Decode`) cannot interfere with one another, similarly
+    for tests. The only exception is `image.RegisterFormat`, but that is
+    mitigated by the points below.
+*   It is extremely unlikely that a user would want to replace a decoder with a
+    [test double], as the PNG decoder exemplifies a case in which our codebase's
+    preference for real objects applies. However, a user would be more likely to
+    replace a decoder with a test double if the decoder statefully interacted
+    with operating system resources (for example, the network).
+*   Collisions in registration are conceivable, though they are probably rare in
+    practice.
+*   The decoders are stateless, idempotent, and pure.
+
+<a id="globals-default-instance"></a>
+
+### Providing a default instance
+
+While not recommended, it is acceptable to provide a simplified API that uses
+package level state if you need to maximize convenience for the user.
+
+Follow the [litmus tests](#globals-litmus-tests) with these guidelines in such
+cases:
+
+1.  The package must offer clients the ability to create isolated instances of
+    package types as [described above](#globals-forms).
+2.  The public APIs that use global state must be a thin proxy to the previous
+    API. A good example of this is
+    [`http.Handle`](https://pkg.go.dev/net/http#Handle) internally calling
+    [`(*http.ServeMux).Handle`](https://pkg.go.dev/net/http#ServeMux.Handle) on
+    the package variable
+    [`http.DefaultServeMux`](https://pkg.go.dev/net/http#DefaultServeMux).
+3.  This package-level API must only be used by [binary build targets], not
+    [libraries], unless the libraries are undertaking a refactoring to support
+    dependency passing. Infrastructure libraries that can be imported by other
+    packages must not rely on package-level state of the packages they import.
+
+    For example, an infrastructure provider implementing a sidecar that is to be
+    shared with other teams using the API from the top should offer an API to
+    accommodate this:
+
+    ```go
+    // Good:
+    package cloudlogger
+
+    func New() *Logger { ... }
+
+    func Register(r *sidecar.Registry, l *Logger) {
+      r.Register("Cloud Logging", l)
+    }
+    ```
+
+4.  This package-level API must [document](#documentation-conventions) and
+    enforce its invariants (for example, at which stage in the program's life it
+    can be called, whether it can be used concurrently). Further, it must
+    provide an API to reset global state to a known-good default (for example,
+    to facilitate testing).
+
+[binary build targets]: https://github.com/bazelbuild/rules_go/blob/master/docs/go/core/rules.md#go_binary
+[libraries]: https://github.com/bazelbuild/rules_go/blob/master/docs/go/core/rules.md#go_library
+
+See also:
+
+*   [Go Tip #36: Enclosing Package-Level State](https://google.github.io/styleguide/go/index.html#gotip)
+*   [Go Tip #80: Dependency Injection Principles](https://google.github.io/styleguide/go/index.html#gotip)

go/decisions.md

@@ -550,8 +550,8 @@ reader experience.
 # Bad:
 // This is a comment paragraph. The length of individual lines doesn't matter in
 Godoc;
-// but the choice of wrapping causes jagged lines on narrow screens or in
+// but the choice of wrapping causes jagged lines on narrow screens or in code
-Critique,
+review,
 // which can be annoying, especially when in a comment block that will wrap
 repeatedly.
 //
@@ -2318,7 +2318,7 @@ Do not export interfaces that the users of the package do not need.
 [real implementation]: best-practices#use-real-transports
 [public API]: https://abseil.io/resources/swe-book/html/ch12.html#test_via_public_apis
 [double types]: https://abseil.io/resources/swe-book/html/ch13.html#techniques_for_using_test_doubles
-[test doubles]: https://abseil.io/resources/swe-book/html/ch13.html#basic_concepts
+[test double]: https://abseil.io/resources/swe-book/html/ch13.html#basic_concepts
 [tott-438]: https://testing.googleblog.com/2017/08/code-health-eliminate-yagni-smells.html
 
 ```go
@@ -2435,8 +2435,8 @@ over time.
 <a id="TOC-ReceiverType"></a>
 
 A [method receiver] can be passed either as a value or a pointer, just as if it
-were a regular function parameter. The choice of which to choose should be based
+were a regular function parameter. The choice between the two is based on which
-on which [method set(s)] the method should be a part of.
+[method set(s)] the method should be a part of.
 
 [method receiver]: https://golang.org/ref/spec#Method_declarations
 [method set(s)]: https://golang.org/ref/spec#Method_sets
@@ -2882,19 +2882,19 @@ See also:
 
 #### Custom contexts
 
-Do not create custom context types or use interfaces other than context in
+Do not create custom context types or use interfaces other than
-function signatures. There are no exceptions to this rule.
+`context.Context` in function signatures. There are no exceptions to this rule.
 
-Imagine if every team had a custom context. Every function call from package P
+Imagine if every team had a custom context. Every function call from package `p`
-to package Q would have to determine how to convert a `PContext` to a
+to package `q` would have to determine how to convert a `p.Context` to a
-`QContext`, for all pairs of packages P and Q. This is impractical and
+`q.Context`, for all pairs of packages `p` and `q`. This is impractical and
 error-prone for humans, and it makes automated refactorings that add context
 parameters nearly impossible.
 
 If you have application data to pass around, put it in a parameter, in the
-receiver, in globals, or in a Context value if it truly belongs there. Creating
+receiver, in globals, or in a `Context` value if it truly belongs there.
-your own Context type is not acceptable since it undermines the ability of the
+Creating your own context type is not acceptable since it undermines the ability
-Go team to make Go programs work properly in production.
+of the Go team to make Go programs work properly in production.
 
 <a id="crypto-rand"></a>
 
@@ -3217,7 +3217,7 @@ It is user-configurable and should serve most comparison needs.
 [language-defined comparisons]: http://golang.org/ref/spec#Comparison_operators
 [`cmp`]: https://pkg.go.dev/github.com/google/go-cmp/cmp
 [`cmp.Equal`]: https://pkg.go.dev/github.com/google/go-cmp/cmp#Equal
-[`cmp.Diff`]: https://pkg.go.dev/github.com/google/go-cmp/cmp/cmp#Diff
+[`cmp.Diff`]: https://pkg.go.dev/github.com/google/go-cmp/cmp#Diff
 [`protocmp.Transform`]: https://pkg.go.dev/google.golang.org/protobuf/testing/protocmp#Transform
 
 Existing code may make use of the following older libraries, and may continue

go/index.md

@@ -157,7 +157,9 @@ reviews.
 *   [Go Interfaces](https://research.swtch.com/interfaces)
 *   [Go Proverbs](https://go-proverbs.github.io/)
 
-*   <a id="gotip"></a> Go tips - stay tuned.
+*   <a id="gotip"></a> Go Tip Episodes - stay tuned.
+
+*   <a id="unit-testing-practices"></a> Unit Testing Practices - stay tuned.
 
 **Relevant Testing-on-the-Toilet articles**