Repository: nats-io/nats-server
Pull request #6164 (2024-11-22 21:19:30)

Facilitate flake tests reproduction with Antithesis
1. Introduce dependency on antithesis-sdk-go (default NOOP variant)
2. Introduce assertions utilities suitable for use in tests
3. Instrument frequently used test utilities with the new assertions

N.B. unless the `enable_antithesis_sdk` tag is specified at build time, both the SDK and the assertions wrappers are NOOP, and should have no effect.

This will also facilitate other Antithesis one off experiments since developers no longer have to manually add the SDK to their branch.

Signed-off-by: Your Name <marco@nats.io>


mprimi: > Antithesis [already produce `default-no-op` builds](https://github.com/antithesishq/antithesis-sdk-go/tags) of their SDK

I'll be honest, i didn't notice this release (it's not documented and wasn't brought up in discussion).
I also assumed (wrongly?) you'd not be comfortable bringing in the full SDK.

But if you are, then I agree using the official client is a great option.

I just launched a side-by-side comparison for the test flakes from last week, we should see the official client find the same violated assertions.

Assuming the two are equivalent in terms of results for that experiment, there's still a few minor differences and tradeoffs worth considering.

This `fallback` implementation gives us more control:
 - Add additional behavior (e.g. stack trace on assert false is useful, even outside Antithesis), and more
 - Control grouping and de-duplication by test (i.e. 2 distinct tests failing on the same `require_*` or `waitFor*` will produce 2 example traces. vs. the `official` client will produce a single trace/example per assertion violation)
 - Control over panic or continue execution on assert violation
 - No surprise breakage when upgrading dependencies

Of course there are upsides of using the `official` library:
 - Less code to own/maintain, simpler upgrades
 - Devs no longer need to `go get antithesis` in their branch when running one-off experiments

One more important distinction:

The `fallback` impl. is specifically for tests, it only exposes `Assert(condition)` and `AssertUnreachable`. Both take a `*testing.T` argument which means we won't end up using it by mistake in server code.

The `official` client is generic and has a much richer API suitable for the entire project, not just tests.
It exposes things like `Reachable`, `Sometimes`, which do not work properly inside a test context**. Devs may use these API count on the results without realizing they do not actually work.

So the richer API is a double-edged sword.


** Antithesis instrumentor skips test code => Assertions in tests code are not catalogued => Antithesis does not know of these assertion ahead of time =>  Antithesis cannot know it failed to reach a `Reachable` statement => There will be no failed `Reachable` property in the report. This can be misleading, as a failed property ins not reported.




Patch:
diff --git a/go.mod b/go.mod
index 61f80759..701fd504 100644
--- a/go.mod
+++ b/go.mod
@@ -5,6 +5,7 @@ go 1.22
 toolchain go1.22.8
 
 require (
+	github.com/antithesishq/antithesis-sdk-go v0.4.3-default-no-op
 	github.com/google/go-tpm v0.9.3
 	github.com/klauspost/compress v1.17.11
 	github.com/minio/highwayhash v1.0.3
diff --git a/go.sum b/go.sum
index fbeed8b5..997403c9 100644
--- a/go.sum
+++ b/go.sum
@@ -1,3 +1,5 @@
+github.com/antithesishq/antithesis-sdk-go v0.4.3-default-no-op h1:+OSa/t11TFhqfrX0EOSqQBDJ0YlpmK0rDSiB19dg9M0=
+github.com/antithesishq/antithesis-sdk-go v0.4.3-default-no-op/go.mod h1:IUpT2DPAKh6i/YhSbt6Gl3v2yvUZjmKncl7U91fup7E=
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/google/go-tpm v0.9.3 h1:+yx0/anQuGzi+ssRqeD6WpXjW2L/V0dItUayO0i9sRc=
diff --git a/internal/antithesis/noop.go b/internal/antithesis/noop.go
new file mode 100644
index 00000000..3255e88a
--- /dev/null
+++ b/internal/antithesis/noop.go
@@ -0,0 +1,27 @@
+// Copyright 2022-2024 The NATS Authors
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// This file is used iff the `enable_antithesis_sdk` build tag is not present
+//go:build !enable_antithesis_sdk
+
+package antithesis
+
+import (
+	"testing"
+)
+
+// AssertUnreachable this implementation is a NOOP
+func AssertUnreachable(_ testing.TB, _ string, _ map[string]any) {}
+
+// Assert this implementation is a NOOP
+func Assert(_ testing.TB, _ bool, _ string, _ map[string]any) {}


Test patch:
diff --git a/internal/antithesis/test_assert.go b/internal/antithesis/test_assert.go
new file mode 100644
index 00000000..23bac3aa
--- /dev/null
+++ b/internal/antithesis/test_assert.go
@@ -0,0 +1,111 @@
+// Copyright 2022-2024 The NATS Authors
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// This file is used iff the `enable_antithesis_sdk` build tag is present
+//go:build enable_antithesis_sdk
+
+package antithesis
+
+import (
+	"encoding/json"
+	"fmt"
+	"runtime/debug"
+	"testing"
+
+	"github.com/antithesishq/antithesis-sdk-go/assert"
+)
+
+// This file provides assertions utility functions suitable for use in tests.
+// It is a thin wrapper around the Antithesis SDK.
+//
+// Notice that unlike other assertions libraries, a violation does not halt execution or fail the test.
+// The effects of violating an assertion, are:
+//  1. Print the violation message, prefixed by the test name in which it happened
+//  2. Print the stack for the calling goroutine so each failed test has a clear trace of the failure
+//  3. Invoke the underlying Antithesis assertion
+//
+// N.B. Enabling this module outside for tests running outside of Antithesis will enable 1 and 2 above, but not 3.
+// therefore it can be useful to output additional test failure details when running tests locally or in CI.
+
+// AssertUnreachable is used to flag code branches that should not get invoked ever.
+// Example:
+//
+// pubAck, err := js.Publish(...)
+//
+//	if err != nil {
+//	    antithesis.AssertUnreachable(t, "Publish failed", map[string]any{"error": err.Error()})
+//	    t.Fatalf("Publish failed with error: %s", err)
+//	}
+func AssertUnreachable(t testing.TB, message string, details map[string]any) {
+	// Always print a message
+	fmt.Printf("{*} [%s] Assert Unreachable violation: %s\n", t.Name(), message)
+	if details != nil && len(details) > 0 {
+		fmt.Printf("{*} Details:\n")
+		jsonDetails, err := json.MarshalIndent(details, "", "  ")
+		if err != nil {
+			panic(err)
+		}
+		fmt.Println(string(jsonDetails))
+	}
+
+	// Always print the stack trace
+	fmt.Printf("{*} Stack trace:\n")
+	debug.PrintStack()
+
+	// N.B. as of today, message (as-is) is the unique identifier of the event
+	// Therefore this will de-duplicate the same assertion failing in 2 different tests
+	// But not the same assertion failing at 2 different lines of the same test
+	messageWithTestName := fmt.Sprintf("[%s] %s", t.Name(), message)
+
+	// Fire assertion violation event (if Antithesis is enabled)
+	assert.Unreachable(messageWithTestName, details)
+}
+
+// Assert is used to check that some given condition is always true,
+// Example:
+//
+//	antithesis.Assert(t, sequence > lastSequence, "Non-monotonic stream sequence number", map[string]any{
+//	    "stream": streamName,
+//	    "connection_id": nc.Id(),
+//	    "sequence": sequence,
+//	    "lastSequence": lastSequence,
+//	})
+func Assert(t testing.TB, condition bool, message string, details map[string]any) {
+	// Condition is true, nothing to do
+	if condition {
+		return
+	}
+
+	// Always print a message
+	fmt.Printf("{*} [%s] Assert violation: %s\n", t.Name(), message)
+	if details != nil && len(details) > 0 {
+		fmt.Printf("{*} Details:\n")
+		jsonDetails, err := json.MarshalIndent(details, "", "  ")
+		if err != nil {
+			panic(err)
+		}
+		fmt.Println(string(jsonDetails))
+	}
+
+	// Always print the stack trace
+	fmt.Printf("{*} Stack trace:\n")
+	debug.PrintStack()
+
+	// N.B. as of today, message (as-is) is the unique identifier of the event
+	// Therefore this will de-duplicate the same assertion failing in 2 different tests
+	// But not the same assertion failing at 2 different lines of the same test
+	messageWithTestName := fmt.Sprintf("[%s] %s", t.Name(), message)
+
+	// Fire assertion violation event (if Antithesis is enabled)
+	assert.AlwaysOrUnreachable(false, messageWithTestName, details)
+}
diff --git a/server/jetstream_helpers_test.go b/server/jetstream_helpers_test.go
index 49cec54c..8a4b190b 100644
--- a/server/jetstream_helpers_test.go
+++ b/server/jetstream_helpers_test.go
@@ -35,6 +35,8 @@ import (
 
 	"github.com/nats-io/nats.go"
 	"golang.org/x/time/rate"
+
+	"github.com/nats-io/nats-server/v2/internal/antithesis"
 )
 
 // Support functions
@@ -104,6 +106,10 @@ func (sc *supercluster) waitOnStreamLeader(account, stream string) {
 		}
 		time.Sleep(100 * time.Millisecond)
 	}
+	antithesis.AssertUnreachable(sc.t, "Timeout in supercluster.waitOnStreamLeader", map[string]any{
+		"account": account,
+		"stream":  stream,
+	})
 	sc.t.Fatalf("Expected a stream leader for %q %q, got none", account, stream)
 }
 
@@ -556,6 +562,7 @@ func (sc *supercluster) waitOnLeader() {
 		}
 		time.Sleep(25 * time.Millisecond)
 	}
+	antithesis.AssertUnreachable(sc.t, "Timeout in supercluster.waitOnStreamLeader", nil)
 	sc.t.Fatalf("Expected a cluster leader, got none")
 }
 
@@ -614,6 +621,7 @@ func (sc *supercluster) waitOnPeerCount(n int) {
 		}
 		time.Sleep(100 * time.Millisecond)
 	}
+	antithesis.AssertUnreachable(sc.t, "Timeout in supercluster.waitOnPeerCount", nil)
 	sc.t.Fatalf("Expected a super cluster peer count of %d, got %d", n, len(leader.JetStreamClusterPeers()))
 }
 
@@ -1305,6 +1313,9 @@ func fetchMsgs(t *testing.T, sub *nats.Subscription, numExpected int, totalWait
 	for start, count, wait := time.Now(), numExpected, totalWait; len(result) != numExpected; {
 		msgs, err := sub.Fetch(count, nats.MaxWait(wait))
 		if err != nil {
+			antithesis.AssertUnreachable(t, "Fetch error", map[string]any{
+				"error": err.Error(),
+			})
 			t.Fatal(err)
 		}
 		result = append(result, msgs...)
@@ -1314,6 +1325,10 @@ func fetchMsgs(t *testing.T, sub *nats.Subscription, numExpected int, totalWait
 		}
 	}
 	if len(result) != numExpected {
+		antithesis.AssertUnreachable(t, "Unexpected fetch messages count", map[string]any{
+			"expected": numExpected,
+			"actual":   len(result),
+		})
 		t.Fatalf("Unexpected msg count, got %d, want %d", len(result), numExpected)
 	}
 	return result
@@ -1364,6 +1379,9 @@ func (c *cluster) waitOnPeerCount(n int) {
 			leader = c.leader()
 		}
 	}
+	antithesis.AssertUnreachable(c.t, "Timeout in cluster.waitOnPeerCount", map[string]any{
+		"cluster": c.name,
+	})
 	c.t.Fatalf("Expected a cluster peer count of %d, got %d", n, len(leader.JetStreamClusterPeers()))
 }
 
@@ -1377,6 +1395,12 @@ func (c *cluster) waitOnConsumerLeader(account, stream, consumer string) {
 		}
 		time.Sleep(100 * time.Millisecond)
 	}
+	antithesis.AssertUnreachable(c.t, "Timeout in cluster.waitOnConsumerLeader", map[string]any{
+		"cluster":  c.name,
+		"account":  account,
+		"stream":   stream,
+		"consumer": consumer,
+	})
 	c.t.Fatalf("Expected a consumer leader for %q %q %q, got none", account, stream, consumer)
 }
 
@@ -1410,6 +1434,11 @@ func (c *cluster) waitOnStreamLeader(account, stream string) {
 		}
 		time.Sleep(100 * time.Millisecond)
 	}
+	antithesis.AssertUnreachable(c.t, "Timeout in cluster.waitOnStreamLeader", map[string]any{
+		"cluster": c.name,
+		"account": account,
+		"stream":  stream,
+	})
 	c.t.Fatalf("Expected a stream leader for %q %q, got none", account, stream)
 }
 
@@ -1443,6 +1472,11 @@ func (c *cluster) waitOnStreamCurrent(s *Server, account, stream string) {
 		}
 		time.Sleep(100 * time.Millisecond)
 	}
+	antithesis.AssertUnreachable(c.t, "Timeout in cluster.waitOnStreamCurrent", map[string]any{
+		"cluster": c.name,
+		"account": account,
+		"stream":  stream,
+	})
 	c.t.Fatalf("Expected server %q to eventually be current for stream %q", s, stream)
 }
 
@@ -1456,6 +1490,10 @@ func (c *cluster) waitOnServerHealthz(s *Server) {
 		}
 		time.Sleep(100 * time.Millisecond)
 	}
+	antithesis.AssertUnreachable(c.t, "Timeout in cluster.waitOnServerHealthz", map[string]any{
+		"cluster": c.name,
+		"server":  s.Name(),
+	})
 	c.t.Fatalf("Expected server %q to eventually return healthz 'ok', but got %q", s, s.healthz(nil).Error)
 }
 
@@ -1468,6 +1506,10 @@ func (c *cluster) waitOnServerCurrent(s *Server) {
 			return
 		}
 	}
+	antithesis.AssertUnreachable(c.t, "Timeout in cluster.waitOnServerCurrent", map[string]any{
+		"cluster": c.name,
+		"server":  s.Name(),
+	})
 	c.t.Fatalf("Expected server %q to eventually be current", s)
 }
 
@@ -1515,6 +1557,9 @@ func (c *cluster) expectNoLeader() {
 		}
 		time.Sleep(20 * time.Millisecond)
 	}
+	antithesis.AssertUnreachable(c.t, "Timeout in cluster.expectNoLeader", map[string]any{
+		"cluster": c.name,
+	})
 	c.t.Fatalf("Expected no leader but have one")
 }
 
@@ -1529,6 +1574,9 @@ func (c *cluster) waitOnLeader() {
 		time.Sleep(10 * time.Millisecond)
 	}
 
+	antithesis.AssertUnreachable(c.t, "Timeout in cluster.waitOnLeader", map[string]any{
+		"cluster": c.name,
+	})
 	c.t.Fatalf("Expected a cluster leader, got none")
 }
 
@@ -1548,6 +1596,9 @@ func (c *cluster) waitOnAccount(account string) {
 		continue
 	}
 
+	antithesis.AssertUnreachable(c.t, "Timeout in cluster.waitOnAccount", map[string]any{
+		"account": account,
+	})
 	c.t.Fatalf("Expected account %q to exist but didn't", account)
 }
 
@@ -1579,6 +1630,9 @@ func (c *cluster) waitOnClusterReadyWithNumPeers(numPeersExpected int) {
 
 	if leader == nil {
 		c.shutdown()
+		antithesis.AssertUnreachable(c.t, "Timeout in cluster.waitOnClusterReadyWithNumPeers (1)", map[string]any{
+			"cluster": c.name,
+		})
 		c.t.Fatalf("Failed to elect a meta-leader")
 	}
 
@@ -1586,8 +1640,16 @@ func (c *cluster) waitOnClusterReadyWithNumPeers(numPeersExpected int) {
 	c.shutdown()
 
 	if leader == nil {
+		antithesis.AssertUnreachable(c.t, "Timeout in cluster.waitOnClusterReadyWithNumPeers (2)", map[string]any{
+			"cluster": c.name,
+		})
 		c.t.Fatalf("Expected a cluster leader and fully formed cluster, no leader")
 	} else {
+		antithesis.AssertUnreachable(c.t, "Timeout in cluster.waitOnClusterReadyWithNumPeers (3)", map[string]any{
+			"cluster":        c.name,
+			"peers_expected": numPeersExpected,
+			"peers_seen":     peersSeen,
+		})
 		c.t.Fatalf("Expected a fully formed cluster, only %d of %d peers seen", peersSeen, numPeersExpected)
 	}
 }
diff --git a/server/server_test.go b/server/server_test.go
index 235c40ad..064db348 100644
--- a/server/server_test.go
+++ b/server/server_test.go
@@ -37,6 +37,7 @@ import (
 
 	"github.com/nats-io/nats.go"
 
+	"github.com/nats-io/nats-server/v2/internal/antithesis"
 	srvlog "github.com/nats-io/nats-server/v2/logger"
 )
 
@@ -57,6 +58,7 @@ func checkFor(t testing.TB, totalWait, sleepDur time.Duration, f func() error) {
 	t.Helper()
 	err := checkForErr(totalWait, sleepDur, f)
 	if err != nil {
+		antithesis.AssertUnreachable(t, "Timeout in checkFor", nil)
 		t.Fatal(err.Error())
 	}
 }
diff --git a/server/test_test.go b/server/test_test.go
index 7985389d..5d720141 100644
--- a/server/test_test.go
+++ b/server/test_test.go
@@ -23,6 +23,8 @@ import (
 	"strings"
 	"testing"
 	"time"
+
+	"github.com/nats-io/nats-server/v2/internal/antithesis"
 )
 
 // DefaultTestOptions are default options for the unit tests.
@@ -58,6 +60,7 @@ func RunRandClientPortServer(t *testing.T) *Server {
 func require_True(t testing.TB, b bool) {
 	t.Helper()
 	if !b {
+		antithesis.AssertUnreachable(t, "Failed require_True check", nil)
 		t.Fatalf("require true, but got false")
 	}
 }
@@ -65,6 +68,7 @@ func require_True(t testing.TB, b bool) {
 func require_False(t testing.TB, b bool) {
 	t.Helper()
 	if b {
+		antithesis.AssertUnreachable(t, "Failed require_False check", nil)
 		t.Fatalf("require false, but got true")
 	}
 }
@@ -72,6 +76,9 @@ func require_False(t testing.TB, b bool) {
 func require_NoError(t testing.TB, err error) {
 	t.Helper()
 	if err != nil {
+		antithesis.AssertUnreachable(t, "Failed require_NoError check", map[string]any{
+			"error": err.Error(),
+		})
 		t.Fatalf("require no error, but got: %v", err)
 	}
 }
@@ -83,6 +90,7 @@ func require_NotNil[T any](t testing.TB, v T) {
 	case reflect.Ptr, reflect.Interface, reflect.Slice,
 		reflect.Map, reflect.Chan, reflect.Func:
 		if r.IsNil() {
+			antithesis.AssertUnreachable(t, "Failed require_NotNil check", nil)
 			t.Fatalf("require not nil, but got nil")
 		}
 	}
@@ -92,6 +100,10 @@ func require_Contains(t *testing.T, s string, subStrs ...string) {
 	t.Helper()
 	for _, subStr := range subStrs {
 		if !strings.Contains(s, subStr) {
+			antithesis.AssertUnreachable(t, "Failed require_Contains check", map[string]any{
+				"string":      s,
+				"sub_strings": subStr,
+			})
 			t.Fatalf("require %q to be contained in %q", subStr, s)
 		}
 	}
@@ -100,6 +112,7 @@ func require_Contains(t *testing.T, s string, subStrs ...string) {
 func require_Error(t testing.TB, err error, expected ...error) {
 	t.Helper()
 	if err == nil {
+		antithesis.AssertUnreachable(t, "Failed require_Error check (nil error)", nil)
 		t.Fatalf("require error, but got none")
 	}
 	if len(expected) == 0 {
@@ -117,12 +130,17 @@ func require_Error(t testing.TB, err error, expected ...error) {
 			return
 		}
 	}
+
+	antithesis.AssertUnreachable(t, "Failed require_Error check (unexpected error)", map[string]any{
+		"error": err.Error(),
+	})
 	t.Fatalf("Expected one of %v, got '%v'", expected, err)
 }
 
 func require_Equal[T comparable](t testing.TB, a, b T) {
 	t.Helper()
 	if a != b {
+		antithesis.AssertUnreachable(t, "Failed require_Equal check", nil)
 		t.Fatalf("require %T equal, but got: %v != %v", a, a, b)
 	}
 }
@@ -130,6 +148,7 @@ func require_Equal[T comparable](t testing.TB, a, b T) {
 func require_NotEqual[T comparable](t testing.TB, a, b T) {
 	t.Helper()
 	if a == b {
+		antithesis.AssertUnreachable(t, "Failed require_NotEqual check", nil)
 		t.Fatalf("require %T not equal, but got: %v == %v", a, a, b)
 	}
 }
@@ -137,6 +156,7 @@ func require_NotEqual[T comparable](t testing.TB, a, b T) {
 func require_Len(t testing.TB, a, b int) {
 	t.Helper()
 	if a != b {
+		antithesis.AssertUnreachable(t, "Failed require_Len check", nil)
 		t.Fatalf("require len, but got: %v != %v", a, b)
 	}
 }
@@ -144,6 +164,7 @@ func require_Len(t testing.TB, a, b int) {
 func require_LessThan[T cmp.Ordered](t *testing.T, a, b T) {
 	t.Helper()
 	if a >= b {
+		antithesis.AssertUnreachable(t, "Failed require_LessThan check", nil)
 		t.Fatalf("require %v to be less than %v", a, b)
 	}
 }
@@ -154,6 +175,7 @@ func require_ChanRead[T any](t *testing.T, ch chan T, timeout time.Duration) T {
 	case v := <-ch:
 		return v
 	case <-time.After(timeout):
+		antithesis.AssertUnreachable(t, "Failed require_ChanRead check", nil)
 		t.Fatalf("require read from channel within %v but didn't get anything", timeout)
 	}
 	panic("this shouldn't be possible")
@@ -163,6 +185,7 @@ func require_NoChanRead[T any](t *testing.T, ch chan T, timeout time.Duration) {
 	t.Helper()
 	select {
 	case <-ch:
+		antithesis.AssertUnreachable(t, "Failed require_NoChanRead check", nil)
 		t.Fatalf("require no read from channel within %v but got something", timeout)
 	case <-time.After(timeout):
 	}