cmd/roachtest: remove the use of runtime.Goexit

[petermattis]

We were abusing runtime.Goexit to kill a goroutine when test.Fatal
was called, but then transforming then aborting the resulting panic into
a panic(errGoexit). This abuse of runtime.Goexit was broken by
go1.14 which makes the panic generated by Goexit truly
unrecoverable (previously it was only documented as so). Now we simply
panic(errGoexit) from the get-go. This opens up the possibility that a
roachtest may recover from this panic, but no current roachtests call
recover().

See #48737

Release note: None

[cockroach-teamcity]

This change is 

[bdarnell]

The message in errGoexit is now a little misleading.

Reviewed 3 of 4 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell, @otan, @petermattis, and @tbg)

---

pkg/cmd/roachtest/test_runner.go, line 596 at r1 (raw file):

		t.end = timeutil.Now()

		if err := recover(); err != nil && err != errGoexit {

If errGoexit reaches this point (I don't think it should), shouldn't we still treat it as a failed test?

---

pkg/cmd/roachtest/test_runner.go, line 747 at r1 (raw file):

		// This is the call to actually run the test.
		defer func() {
			if r := recover(); r != nil && r != errGoexit {

Ditto.

[tbg]

To be fair, we weren't strictly abusing runtime.Goexit but rather trying to stay true to *testing.T. (There's no question that then suppressing the Goexit in some goroutines is a hack). I wish *testing.T would also stop using Goexit, but that won't ever happen.

[tbg]

This message is now misleading

[tbg]

Just for my sanity, can you run a local roachtest that you've changed to t.Fatal (on the main goroutine) and make sure that the test failure will propagate properly?

[tbg]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell, @otan, and @petermattis)

---

pkg/cmd/roachtest/test_runner.go, line 596 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

If errGoexit reaches this point (I don't think it should), shouldn't we still treat it as a failed test?

If errGoexit reaches this point, t.mu.failed is already true. But I had to look at this as well to figure this out. Worthy of a comment.

[petermattis]

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

---

pkg/cmd/roachtest/cluster_test.go, line 248 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

This message is now misleading

Heh, that didn't bother me, but since you noticed I renamed errGoexit to errTestFatal and changed the message.

---

pkg/cmd/roachtest/test_runner.go, line 596 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

If errGoexit reaches this point, t.mu.failed is already true. But I had to look at this as well to figure this out. Worthy of a comment.

Ditto what @tbg said. I'll add a comment.

---

pkg/cmd/roachtest/test_runner.go, line 747 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Ditto.

Done.

[petermattis]

Just for my sanity, can you run a local roachtest that you've changed to t.Fatal (on the main goroutine) and make sure that the test failure will propagate properly?

Sure. I added a t.Fatalf("testing failure") to the beginning of the kv* roachtests. Running it locally produced:

...
--- FAIL: kv0/enc=false/nodes=1 (0.70s)
test artifacts and logs in: artifacts/kv0/enc=false/nodes=1/run_1
        kv.go:57,kv.go:185,test_runner.go:757: testing failure
...

Taking the same change and running it on master produced identical looking behavior and output.

[petermattis]

TFTR! CI finally passed.

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)