[fix] Refactor some flakey tests (#10777)

* [fix] Fix flakey test by avoiding full table lock

* [fix] Refactor some flakey tests

- Stop using Eventually() in SessionReaper tests, and use a channel to
  signal when the reaper has run instead.
- Start using random users where we previously used APIEmailAdmin.
- Remove a full table lock and replace it with a selective delete which
  should reduce some lock contention

* Clean up user/session methods

core/cmd/shell_remote_test.go

@@ -257,17 +257,20 @@ func TestShell_DestroyExternalInitiator_NotFound(t *testing.T) {
 func TestShell_RemoteLogin(t *testing.T) {
 
 	app := startNewApplicationV2(t, nil)
+	orm := app.SessionORM()
+
+	u := cltest.NewUserWithSession(t, orm)
 
 	tests := []struct {
 		name, file string
 		email, pwd string
 		wantError  bool
 	}{
-		{"success prompt", "", cltest.APIEmailAdmin, cltest.Password, false},
+		{"success prompt", "", u.Email, cltest.Password, false},
 		{"success file", "../internal/fixtures/apicredentials", "", "", false},
 		{"failure prompt", "", "[email protected]", "wrongpwd", true},
 		{"failure file", "/tmp/doesntexist", "", "", true},
-		{"failure file w correct prompt", "/tmp/doesntexist", cltest.APIEmailAdmin, cltest.Password, true},
+		{"failure file w correct prompt", "/tmp/doesntexist", u.Email, cltest.Password, true},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
@@ -297,7 +300,8 @@ func TestShell_RemoteBuildCompatibility(t *testing.T) {
 	t.Parallel()
 
 	app := startNewApplicationV2(t, nil)
-	enteredStrings := []string{cltest.APIEmailAdmin, cltest.Password}
+	u := cltest.NewUserWithSession(t, app.SessionORM())
+	enteredStrings := []string{u.Email, cltest.Password}
 	prompter := &cltest.MockCountingPrompter{T: t, EnteredStrings: append(enteredStrings, enteredStrings...)}
 	client := app.NewAuthenticatingShell(prompter)
 
@@ -335,6 +339,7 @@ func TestShell_CheckRemoteBuildCompatibility(t *testing.T) {
 	t.Parallel()
 
 	app := startNewApplicationV2(t, nil)
+	u := cltest.NewUserWithSession(t, app.SessionORM())
 	tests := []struct {
 		name                         string
 		remoteVersion, remoteSha     string
@@ -349,7 +354,7 @@ func TestShell_CheckRemoteBuildCompatibility(t *testing.T) {
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			enteredStrings := []string{cltest.APIEmailAdmin, cltest.Password}
+			enteredStrings := []string{u.Email, cltest.Password}
 			prompter := &cltest.MockCountingPrompter{T: t, EnteredStrings: enteredStrings}
 			client := app.NewAuthenticatingShell(prompter)
 
@@ -410,8 +415,9 @@ func TestShell_ChangePassword(t *testing.T) {
 	t.Parallel()
 
 	app := startNewApplicationV2(t, nil)
+	u := cltest.NewUserWithSession(t, app.SessionORM())
 
-	enteredStrings := []string{cltest.APIEmailAdmin, cltest.Password}
+	enteredStrings := []string{u.Email, cltest.Password}
 	prompter := &cltest.MockCountingPrompter{T: t, EnteredStrings: enteredStrings}
 
 	client := app.NewAuthenticatingShell(prompter)
@@ -459,7 +465,8 @@ func TestShell_Profile_InvalidSecondsParam(t *testing.T) {
 	t.Parallel()
 
 	app := startNewApplicationV2(t, nil)
-	enteredStrings := []string{cltest.APIEmailAdmin, cltest.Password}
+	u := cltest.NewUserWithSession(t, app.SessionORM())
+	enteredStrings := []string{u.Email, cltest.Password}
 	prompter := &cltest.MockCountingPrompter{T: t, EnteredStrings: enteredStrings}
 
 	client := app.NewAuthenticatingShell(prompter)
@@ -489,7 +496,8 @@ func TestShell_Profile(t *testing.T) {
 	t.Parallel()
 
 	app := startNewApplicationV2(t, nil)
-	enteredStrings := []string{cltest.APIEmailAdmin, cltest.Password}
+	u := cltest.NewUserWithSession(t, app.SessionORM())
+	enteredStrings := []string{u.Email, cltest.Password}
 	prompter := &cltest.MockCountingPrompter{T: t, EnteredStrings: enteredStrings}
 
 	client := app.NewAuthenticatingShell(prompter)
@@ -656,7 +664,7 @@ func TestShell_AutoLogin(t *testing.T) {
 	require.NoError(t, err)
 
 	// Expire the session and then try again
-	pgtest.MustExec(t, app.GetSqlxDB(), "TRUNCATE sessions")
+	pgtest.MustExec(t, app.GetSqlxDB(), "delete from sessions where email = $1", user.Email)
 	err = client.ListJobs(cli.NewContext(nil, fs, nil))
 	require.NoError(t, err)
 }

core/cmd/shell_test.go

@@ -33,13 +33,16 @@ import (
 func TestTerminalCookieAuthenticator_AuthenticateWithoutSession(t *testing.T) {
 	t.Parallel()
 
+	app := cltest.NewApplicationEVMDisabled(t)
+	u := cltest.NewUserWithSession(t, app.SessionORM())
+
 	tests := []struct {
 		name, email, pwd string
 	}{
 		{"bad email", "notreal", cltest.Password},
-		{"bad pwd", cltest.APIEmailAdmin, "mostcommonwrongpwdever"},
+		{"bad pwd", u.Email, "mostcommonwrongpwdever"},
 		{"bad both", "notreal", "mostcommonwrongpwdever"},
-		{"correct", cltest.APIEmailAdmin, cltest.Password},
+		{"correct", u.Email, cltest.Password},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
@@ -63,14 +66,16 @@ func TestTerminalCookieAuthenticator_AuthenticateWithSession(t *testing.T) {
 	app := cltest.NewApplicationEVMDisabled(t)
 	require.NoError(t, app.Start(testutils.Context(t)))
 
+	u := cltest.NewUserWithSession(t, app.SessionORM())
+
 	tests := []struct {
 		name, email, pwd string
 		wantError        bool
 	}{
 		{"bad email", "notreal", cltest.Password, true},
-		{"bad pwd", cltest.APIEmailAdmin, "mostcommonwrongpwdever", true},
+		{"bad pwd", u.Email, "mostcommonwrongpwdever", true},
 		{"bad both", "notreal", "mostcommonwrongpwdever", true},
-		{"success", cltest.APIEmailAdmin, cltest.Password, false},
+		{"success", u.Email, cltest.Password, false},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {

core/internal/cltest/mocks.go

@@ -27,6 +27,7 @@ import (
 	gethTypes "github.com/ethereum/go-ethereum/core/types"
 	"github.com/robfig/cron/v3"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 // MockSubscription a mock subscription
@@ -308,35 +309,16 @@ func MustRandomUser(t testing.TB) sessions.User {
 	return r
 }
 
-// CreateUserWithRole inserts a new user with specified role and associated test DB email into the test DB
-func CreateUserWithRole(t testing.TB, role sessions.UserRole) sessions.User {
-	email := ""
-	switch role {
-	case sessions.UserRoleAdmin:
-		email = APIEmailAdmin
-	case sessions.UserRoleEdit:
-		email = APIEmailEdit
-	case sessions.UserRoleRun:
-		email = APIEmailRun
-	case sessions.UserRoleView:
-		email = APIEmailViewOnly
-	default:
-		t.Fatal("Unexpected role for CreateUserWithRole")
-	}
-
-	r, err := sessions.NewUser(email, Password, role)
-	if err != nil {
-		logger.TestLogger(t).Panic(err)
-	}
-	return r
-}
+func NewUserWithSession(t testing.TB, orm sessions.ORM) sessions.User {
+	u := MustRandomUser(t)
+	require.NoError(t, orm.CreateUser(&u))
 
-func MustNewUser(t *testing.T, email, password string) sessions.User {
-	r, err := sessions.NewUser(email, password, sessions.UserRoleAdmin)
-	if err != nil {
-		t.Fatal(err)
-	}
-	return r
+	_, err := orm.CreateSession(sessions.SessionRequest{
+		Email:    u.Email,
+		Password: Password,
+	})
+	require.NoError(t, err)
+	return u
 }
 
 type MockAPIInitializer struct {

core/sessions/orm_test.go

@@ -34,8 +34,8 @@ func TestORM_FindUser(t *testing.T) {
 	t.Parallel()
 
 	db, orm := setupORM(t)
-	user1 := cltest.MustNewUser(t, "[email protected]", cltest.Password)
-	user2 := cltest.MustNewUser(t, "[email protected]", cltest.Password)
+	user1 := cltest.MustRandomUser(t)
+	user2 := cltest.MustRandomUser(t)
 
 	require.NoError(t, orm.CreateUser(&user1))
 	require.NoError(t, orm.CreateUser(&user2))
@@ -56,20 +56,19 @@ func TestORM_AuthorizedUserWithSession(t *testing.T) {
 		sessionID       string
 		sessionDuration time.Duration
 		wantError       string
-		wantEmail       string
 	}{
-		{"authorized", "correctID", cltest.MustParseDuration(t, "3m"), "", "have@email"},
-		{"expired", "correctID", cltest.MustParseDuration(t, "0m"), sessions.ErrUserSessionExpired.Error(), ""},
-		{"incorrect", "wrong", cltest.MustParseDuration(t, "3m"), sessions.ErrUserSessionExpired.Error(), ""},
-		{"empty", "", cltest.MustParseDuration(t, "3m"), sessions.ErrEmptySessionID.Error(), ""},
+		{"authorized", "correctID", cltest.MustParseDuration(t, "3m"), ""},
+		{"expired", "correctID", cltest.MustParseDuration(t, "0m"), sessions.ErrUserSessionExpired.Error()},
+		{"incorrect", "wrong", cltest.MustParseDuration(t, "3m"), sessions.ErrUserSessionExpired.Error()},
+		{"empty", "", cltest.MustParseDuration(t, "3m"), sessions.ErrEmptySessionID.Error()},
 	}
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			db := pgtest.NewSqlxDB(t)
 			orm := sessions.NewORM(db, test.sessionDuration, logger.TestLogger(t), pgtest.NewQConfig(true), &audit.AuditLoggerService{})
 
-			user := cltest.MustNewUser(t, "have@email", cltest.Password)
+			user := cltest.MustRandomUser(t)
 			require.NoError(t, orm.CreateUser(&user))
 
 			prevSession := cltest.NewSession("correctID")
@@ -83,7 +82,7 @@ func TestORM_AuthorizedUserWithSession(t *testing.T) {
 				require.EqualError(t, err, test.wantError)
 			} else {
 				require.NoError(t, err)
-				assert.Equal(t, test.wantEmail, actual.Email)
+				assert.Equal(t, user.Email, actual.Email)
 				var bumpedSession sessions.Session
 				err = db.Get(&bumpedSession, "SELECT * FROM sessions WHERE ID = $1", prevSession.ID)
 				require.NoError(t, err)
@@ -97,13 +96,13 @@ func TestORM_DeleteUser(t *testing.T) {
 	t.Parallel()
 	_, orm := setupORM(t)
 
-	_, err := orm.FindUser(cltest.APIEmailAdmin)
-	require.NoError(t, err)
+	u := cltest.MustRandomUser(t)
+	require.NoError(t, orm.CreateUser(&u))
 
-	err = orm.DeleteUser(cltest.APIEmailAdmin)
+	err := orm.DeleteUser(u.Email)
 	require.NoError(t, err)
 
-	_, err = orm.FindUser(cltest.APIEmailAdmin)
+	_, err = orm.FindUser(u.Email)
 	require.Error(t, err)
 }
 
@@ -112,14 +111,17 @@ func TestORM_DeleteUserSession(t *testing.T) {
 
 	db, orm := setupORM(t)
 
+	u := cltest.MustRandomUser(t)
+	require.NoError(t, orm.CreateUser(&u))
+
 	session := sessions.NewSession()
-	_, err := db.Exec("INSERT INTO sessions (id, email, last_used, created_at) VALUES ($1, $2, now(), now())", session.ID, cltest.APIEmailAdmin)
+	_, err := db.Exec("INSERT INTO sessions (id, email, last_used, created_at) VALUES ($1, $2, now(), now())", session.ID, u.Email)
 	require.NoError(t, err)
 
 	err = orm.DeleteUserSession(session.ID)
 	require.NoError(t, err)
 
-	_, err = orm.FindUser(cltest.APIEmailAdmin)
+	_, err = orm.FindUser(u.Email)
 	require.NoError(t, err)
 
 	sessions, err := orm.Sessions(0, 10)
@@ -130,14 +132,17 @@ func TestORM_DeleteUserSession(t *testing.T) {
 func TestORM_DeleteUserCascade(t *testing.T) {
 	db, orm := setupORM(t)
 
+	u := cltest.MustRandomUser(t)
+	require.NoError(t, orm.CreateUser(&u))
+
 	session := sessions.NewSession()
-	_, err := db.Exec("INSERT INTO sessions (id, email, last_used, created_at) VALUES ($1, $2, now(), now())", session.ID, cltest.APIEmailAdmin)
+	_, err := db.Exec("INSERT INTO sessions (id, email, last_used, created_at) VALUES ($1, $2, now(), now())", session.ID, u.Email)
 	require.NoError(t, err)
 
-	err = orm.DeleteUser(cltest.APIEmailAdmin)
+	err = orm.DeleteUser(u.Email)
 	require.NoError(t, err)
 
-	_, err = orm.FindUser(cltest.APIEmailAdmin)
+	_, err = orm.FindUser(u.Email)
 	require.Error(t, err)
 
 	sessions, err := orm.Sessions(0, 10)

core/sessions/reaper.go

@@ -13,6 +13,10 @@ type sessionReaper struct {
 	db     *sql.DB
 	config SessionReaperConfig
 	lggr   logger.Logger
+
+	// Receive from this for testing via sr.RunSignal()
+	// to be notified after each reaper run.
+	runSignal chan struct{}
 }
 
 type SessionReaperConfig interface {
@@ -22,11 +26,18 @@ type SessionReaperConfig interface {
 
 // NewSessionReaper creates a reaper that cleans stale sessions from the store.
 func NewSessionReaper(db *sql.DB, config SessionReaperConfig, lggr logger.Logger) utils.SleeperTask {
-	return utils.NewSleeperTask(&sessionReaper{
+	return utils.NewSleeperTask(NewSessionReaperWorker(db, config, lggr))
+}
+
+func NewSessionReaperWorker(db *sql.DB, config SessionReaperConfig, lggr logger.Logger) *sessionReaper {
+	return &sessionReaper{
 		db,
 		config,
 		lggr.Named("SessionReaper"),
-	})
+
+		// For testing only.
+		make(chan struct{}, 10),
+	}
 }
 
 func (sr *sessionReaper) Name() string {
@@ -40,6 +51,11 @@ func (sr *sessionReaper) Work() {
 	if err != nil {
 		sr.lggr.Error("unable to reap stale sessions: ", err)
 	}
+
+	select {
+	case sr.runSignal <- struct{}{}:
+	default:
+	}
 }
 
 // DeleteStaleSessions deletes all sessions before the passed time.

core/sessions/reaper_helper_test.go

@@ -0,0 +1,5 @@
+package sessions
+
+func (sr *sessionReaper) RunSignal() <-chan struct{} {
+	return sr.runSignal
+}