Reduce TestInitDatabaseService flakiness
#49477
Conversation
gabrielcorado
added
the
no-changelog
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
| @@ -1842,6 +1847,9 @@ func TestInitDatabaseService(t *testing.T) { | |||
| exitPayload, ok := event.Payload.(ExitEventPayload) | |||
| require.True(t, ok, "expected ExitEventPayload but got %T", event.Payload) | |||
| require.Equal(t, "db.init", exitPayload.Service.Name()) | |||
| // Database service init is a critical service, meaning failures on | |||
| // it should cause the process to exit with error. | |||
| require.Error(t, eg.Wait()) | |||
There was a problem hiding this comment.
If the requires above fails what will make process stop? Would this line block up to 20s in that case?
(If you take my comment about the t.Cleanup I think this gets solved too.)
|
|
||
| if !test.expectErr { | ||
| _, err := process.WaitForEvent(ctx, TeleportReadyEvent) | ||
| require.NoError(t, err) | ||
| require.NoError(t, process.Close()) | ||
| // Expect Teleport to shutdown without reporting any issue. | ||
| require.NoError(t, eg.Wait()) |
There was a problem hiding this comment.
Should this be a t.Cleanup so it runs even if the assertions before it fail?
(Same for the Wait in the "error" code branch.)
Ie:
t.Cleanup(func() {
cancel()
assert.NoError(t, eg.Wait(), "eg.Wait errored")
})There was a problem hiding this comment.
I agree that we should always call the eg.Wait() to ensure the Teleport state regardless of the assertions' results.
Just a brief context on this change: The main reason for moving this away from t.Cleanup was that the assertion on the WaitForSignals (which is on the errgroup) differs depending on the value on test.expectErr. But I guess having two t.Cleanup definitions per "test branch" is okay. I'll update the code, thanks!
There was a problem hiding this comment.
The Cleanup doesn't need to care about the WaitForSignals, right? Just the cancel/Wait?
There was a problem hiding this comment.
Yes, I mean that the WaitForSignals is done on the errgroup goroutine so that it will be the error reported by eg.Wait
public-teleport-github-review-bot
bot
removed the request for review
from rosstimothy
|
@gabrielcorado is this good to go? |
gabrielcorado
deleted the
gabrielcorado/improve-test-init-database-service-assertions
branch
|
@gabrielcorado See the table below for backport results.
|
Related to #42963.
The problem is likely related to the test cleanup (which removes the temp dir) that happens before the Teleport process entirely shuts down.
This PR updates the test to use the
WaitForSignalsand wait for it to return before letting the test clean up. This ensures the Teleport process is closed in both cases (with and without errors). (This function is the same as when we start the Teleport process from CLI.)In addition, the assertion about the server shutdown has been moved to the test body function (instead of the
t.Cleanupfunction).