Remove multiTestContext

[lunevalex]

This pull request is broken up into two commits:

Makes progress on #8299

This change converts the last set of tests in client_metrics_test,
client_raft_test, client_raft_log_queue_test to use TestCluster instead
of multiTestContext.

Closes #8299

With all the tests converted to use TestCluster/TestServer,
we can finally remove multiTestContext and all of the dependent
code.

Release Note: None

[cockroach-teamcity]

This change is 

[tbg]

Fantastic. I truly didn't think it was going to happen.
Let me know if there are areas of the rewritten tests that you want scrutinized.

Reviewed 6 of 6 files at r1, 3 of 3 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @lunevalex)

---

pkg/kv/kvserver/client_metrics_test.go, line 81 at r1 (raw file):

	}

	wg.Add(numStores)

Did you determine that stopping the stores wasn't necessary?

---

pkg/kv/kvserver/client_metrics_test.go, line 241 at r1 (raw file):

					{
						InMemory: true,
						Size: base.SizeSpec{

Can you add a comment here? Right now this looks like you're setting this up manually when the defaults would serve you just as well, which I assume is false.

---

pkg/kv/kvserver/client_metrics_test.go, line 264 at r1 (raw file):

	}

	// We skip verifying stats on Server[0] because there is no reliable way to

I might be missing something but where are you actually skipping it?

---

pkg/kv/kvserver/client_metrics_test.go, line 265 at r1 (raw file):

	// We skip verifying stats on Server[0] because there is no reliable way to
	// do that given all oif the system table activity generated by the TestCluster.

oif

---

pkg/kv/kvserver/client_raft_test.go, line 470 at r1 (raw file):

	// The filter is very aggressive, so we need to turn it on after the cluster
	// starts up.

The filter avoids all commits? That seems over the top. Just check et.InternalCommitTrigger != nil && et.InternalCommitTrigger.ChangeReplicasTrigger != nil in addition to what's there.

---

pkg/kv/kvserver/client_test.go, line 1653 at r1 (raw file):

}

// This test is here to please the unused code linter, and will be removed in

FYI the usual way we do this is

var _ = (*unusedStruct)(nil)
var _ = (&someStruct{}).unusedMethod

etc (no need to change things here, this works)

[lunevalex]

The two tests that gave me problems were TestStoreMetrics and TestSnapshotAfterTruncationWithUncommittedTail

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @lunevalex, and @tbg)

---

pkg/kv/kvserver/client_metrics_test.go, line 81 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Did you determine that stopping the stores wasn't necessary?

The original comments, claimed it was to reduce the possibility that something would update the metrics while the measurements were being taken. With TestCluster I am mitigating this by only measuring s2 and s3, which dont have any system tables/ranges on them. The other issues is TestCluster.Restart() completely rebuilds the whole store, so metrics are wiped between restarts, which seems problematic for this test.

---

pkg/kv/kvserver/client_metrics_test.go, line 241 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Can you add a comment here? Right now this looks like you're setting this up manually when the defaults would serve you just as well, which I assume is false.

Good catch, it is needed to enable the BlockCache in Pebble

---

pkg/kv/kvserver/client_metrics_test.go, line 264 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

I might be missing something but where are you actually skipping it?

it's the verifyStats() methods bellow, I moved the comment closer to that code

---

pkg/kv/kvserver/client_raft_test.go, line 470 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

The filter avoids all commits? That seems over the top. Just check et.InternalCommitTrigger != nil && et.InternalCommitTrigger.ChangeReplicasTrigger != nil in addition to what's there.

Ack

[tbg]

Reviewed 3 of 6 files at r3, 3 of 3 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @lunevalex)

---

pkg/kv/kvserver/client_metrics_test.go, line 54 at r4 (raw file):

	t.Helper()
	var stores []*kvserver.Store
	// We need to stop the stores at the given indexes, while keeping the reference to the

Stale comment, and while you're here also give verifyStats a comment about the pitfalls of using it on a general running server.

[lunevalex]

bors r=tbg

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[andreimatei]

Reviewable status: complete! 0 of 0 LGTMs obtained

---

pkg/util/leaktest/leaktest.go line 97 at r5 (raw file):

// goroutines leaked.
func AfterTest(t testing.TB) func() {
	// Try a best effort GC to help the race tests move along.

Do you remember why this was necessary?

[tbg]

@andreimatei if memory serves correctly this was all band-aid to try to make #61120 better, but we fixed this properly in the end so hopefully (read: I'm confident that) this runtime.GC() can just be removed.