Improve error handling in Connect gateway integration test

[ravicious]

A few small things that I missed when I added this test in #17950.

mockTSHDEventsClient will most likely be called outside of the test goroutine, so we shouldn't use require.NoError there.

helpers.SetupUserCreds returns an error which I did not inspect. The linter didn't warn me about it for some reason.

[espadolini]

Is this better than using assert.NoError instead?

[ravicious]

I suppose assert.NoError would be better at communicating where exactly the failure has happened.

If I understand correctly, assert.NoError continues the execution if the error is present but the test will ultimately fail. Does this mean that the implementation of Relogin should take the return value of assert.NoError into account?

That is, I'm not sure if I should do this:

	err = helpers.SetupUserCreds(c.tc, c.pack.Root.Cluster.Config.Proxy.SSHAddr.Addr, *creds)
	assert.NoError(c.t, err)

	return &api.ReloginResponse{}, nil

or

	err = helpers.SetupUserCreds(c.tc, c.pack.Root.Cluster.Config.Proxy.SSHAddr.Addr, *creds)
	if !assert.NoError(c.t, err) {
		return nil, trace.Wrap(err)
	}

	return &api.ReloginResponse{}, nil

From what I see, in the project we seem to use the first approach.

[ravicious]

This also reminds me that asserting something without failing the test immediately would also be helpful when checking the number of calls to the mock. I had a test that was failing on this check:

teleport/integration/proxy/teleterm_test.go

Lines 155 to 158 in b2e1807

	client, err = mysql.MakeTestClientWithoutTLS(
	net.JoinHostPort(gateway.LocalAddress(), gateway.LocalPort()),
	gateway.RouteToDatabase())
	require.NoError(t, err)

but if these two were performed as assertions before that require.NoError, it'd have made debugging much easier:

teleport/integration/proxy/teleterm_test.go

Lines 168 to 171 in b2e1807

	require.Equal(t, 1, tshdEventsClient.callCounts["Relogin"],
	"Unexpected number of calls to TSHDEventsClient.Relogin")
	require.Equal(t, 0, tshdEventsClient.callCounts["SendNotification"],
	"Unexpected number of calls to TSHDEventsClient.SendNotification")

Would that be an okay approach or would you rather say it's misusing the assert package?

[ravicious]

@espadolini Could you offer an advice or an opinion on using assert.NoError inside a mocked function vs returning an error from that function? #19145 (comment)

[espadolini]

I don't know if mocks should have access to the *testing.T, that seems kind of an overreach. The testing environment should set up and control the mock instead, so I think the change here is good.

[github-actions]

@ravicious See the table below for backport results.

Branch	Result
branch/v11	Create PR