From f28ae31bbf9100cdef695ee687e4d306cf9e62bd Mon Sep 17 00:00:00 2001
From: Andrei Matei <andrei@cockroachlabs.com>
Date: Tue, 5 Feb 2019 19:24:34 -0800
Subject: [PATCH] server: rework TestClusterVersionBootstrapStrict

This test... I'm not entirely sure what it was supposed to test to be
honest, but it seemed to be more complicated than it needed to be. It
forced and emphasized MinSupportedVersion being equal to
BinaryServerVersion (which is generally not a thing). I've simplified
it, making it not muck with the versions, while keep (I think) the
things it was testing (to the extent that it was testing anything).

This test was also in my way because it created servers that pretended
to be versions that are not technically supported by the binary, and
this kind of funkiness is making my life hard as I'm trying to rework
the way in which versions are propagated and what knobs servers have,
etc.

Release note: None
---
 pkg/server/version_cluster_test.go | 59 ++++++++++++++----------------
 1 file changed, 27 insertions(+), 32 deletions(-)

diff --git a/pkg/server/version_cluster_test.go b/pkg/server/version_cluster_test.go
index ade1852610b4..afca6ccfd360 100644
--- a/pkg/server/version_cluster_test.go
+++ b/pkg/server/version_cluster_test.go
@@ -17,6 +17,7 @@ package server_test
 import (
 	"context"
 	gosql "database/sql"
+	"fmt"
 	"path/filepath"
 	"strconv"
 	"sync/atomic"
@@ -312,44 +313,38 @@ func TestClusterVersionUpgrade(t *testing.T) {
 	}
 }
 
-func TestClusterVersionBootstrapStrict(t *testing.T) {
+// Test that, after cluster bootstrap, the different ways of getting the cluster
+// version all agree.
+func TestAllVersionsAgree(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	ctx := context.Background()
 
-	// Four nodes that are all strictly version X without accepting anything else.
-	for _, versions := range [][][2]string{
-		{{"1.1", "1.1"}, {"1.1", "1.1"}, {"1.1", "1.1"}, {"1.1", "1.1"}},
-		{{"4.7", "4.7"}, {"4.7", "4.7"}, {"4.7", "4.7"}, {"4.7", "4.7"}},
-	} {
-		func() {
-			bootstrapVersion := cluster.ClusterVersion{
-				Version: roachpb.MustParseVersion(versions[0][0]),
-			}
-
-			knobs := base.TestingKnobs{
-				Store: &storage.StoreTestingKnobs{
-					BootstrapVersion: &bootstrapVersion,
-				},
-			}
-			tc := setupMixedCluster(t, knobs, versions, "")
-			defer tc.Stopper().Stop(ctx)
+	tcRaw := testcluster.StartTestCluster(t, 3, base.TestClusterArgs{})
+	defer tcRaw.Stopper().Stop(ctx)
+	tc := testClusterWithHelpers{
+		T:           t,
+		TestCluster: tcRaw,
+	}
 
-			exp := versions[0][0]
+	exp := cluster.BinaryServerVersion.String()
 
-			for i := 0; i < tc.NumServers(); i++ {
-				if version := tc.getVersionFromSetting(i).Version().Version.String(); version != exp {
-					t.Fatalf("%d: incorrect version %s (wanted %s)", i, version, exp)
-				}
-				if version := tc.getVersionFromShow(i); version != exp {
-					t.Fatalf("%d: incorrect version %s (wanted %s)", i, version, exp)
-				}
-
-				if version := tc.getVersionFromSelect(i); version != exp {
-					t.Fatalf("%d: incorrect version %q (wanted %s)", i, version, exp)
-				}
+	// The node bootstrapping the cluster starts at BinaryServerVersion, the
+	// others start at MinimumSupportedVersion and it takes them a gossip update
+	// to get to BinaryServerVersion. Hence, we loop until that gossip comes.
+	testutils.SucceedsSoon(tc, func() error {
+		for i := 0; i < tc.NumServers(); i++ {
+			if version := tc.getVersionFromSetting(i).Version().Version.String(); version != exp {
+				return fmt.Errorf("%d: incorrect version %s (wanted %s)", i, version, exp)
 			}
-		}()
-	}
+			if version := tc.getVersionFromShow(i); version != exp {
+				return fmt.Errorf("%d: incorrect version %s (wanted %s)", i, version, exp)
+			}
+			if version := tc.getVersionFromSelect(i); version != exp {
+				return fmt.Errorf("%d: incorrect version %q (wanted %s)", i, version, exp)
+			}
+		}
+		return nil
+	})
 }
 
 func TestClusterVersionMixedVersionTooOld(t *testing.T) {