diff --git a/server/.golangci.yml b/server/.golangci.yml index ac12198a1f..1770eb2f2b 100644 --- a/server/.golangci.yml +++ b/server/.golangci.yml @@ -143,7 +143,6 @@ issues: channels/store/storetest/team_store.go|\ channels/store/storetest/thread_store.go|\ channels/store/storetest/user_store.go|\ - channels/web/oauth_test.go|\ channels/web/web_test.go|\ cmd/mattermost/commands/cmdtestlib.go|\ cmd/mattermost/commands/db.go|\ diff --git a/server/channels/api4/file_test.go b/server/channels/api4/file_test.go index cf88da84a6..3159e39ae3 100644 --- a/server/channels/api4/file_test.go +++ b/server/channels/api4/file_test.go @@ -59,11 +59,11 @@ func fileBytes(t *testing.T, path string) []byte { return bb } -func testDoUploadFileRequest(t testing.TB, c *model.Client4, url string, blob []byte, contentType string, +func testDoUploadFileRequest(tb testing.TB, c *model.Client4, url string, blob []byte, contentType string, contentLength int64, ) (*model.FileUploadResponse, *model.Response, error) { req, err := http.NewRequest("POST", c.APIURL+"/files"+url, bytes.NewReader(blob)) - require.NoError(t, err) + require.NoError(tb, err) if contentLength != 0 { req.ContentLength = contentLength @@ -74,8 +74,8 @@ func testDoUploadFileRequest(t testing.TB, c *model.Client4, url string, blob [] } resp, err := c.HTTPClient.Do(req) - require.NoError(t, err) - require.NotNil(t, resp) + require.NoError(tb, err) + require.NotNil(tb, resp) defer closeBody(resp) if resp.StatusCode >= 300 { @@ -90,7 +90,7 @@ func testDoUploadFileRequest(t testing.TB, c *model.Client4, url string, blob [] } func testUploadFilesPost( - t testing.TB, + tb testing.TB, c *model.Client4, channelId string, names []string, @@ -102,9 +102,9 @@ func testUploadFilesPost( // Do not check len(clientIds), leave it entirely to the user to // provide. The server will error out if it does not match the number // of files, but it's not critical here. - require.NotEmpty(t, names) - require.NotEmpty(t, blobs) - require.Equal(t, len(names), len(blobs)) + require.NotEmpty(tb, names) + require.NotEmpty(tb, blobs) + require.Equal(tb, len(names), len(blobs)) fileUploadResponse := &model.FileUploadResponse{} for i, blob := range blobs { @@ -126,7 +126,7 @@ func testUploadFilesPost( postURL += "&bookmark=true" } - fur, resp, err := testDoUploadFileRequest(t, c, postURL, blob, ct, cl) + fur, resp, err := testDoUploadFileRequest(tb, c, postURL, blob, ct, cl) if err != nil { return nil, resp, err } @@ -145,7 +145,7 @@ func testUploadFilesPost( } func testUploadFilesMultipart( - t testing.TB, + tb testing.TB, c *model.Client4, channelId string, names []string, @@ -160,21 +160,21 @@ func testUploadFilesMultipart( // Do not check len(clientIds), leave it entirely to the user to // provide. The server will error out if it does not match the number // of files, but it's not critical here. - require.NotEmpty(t, names) - require.NotEmpty(t, blobs) - require.Equal(t, len(names), len(blobs)) + require.NotEmpty(tb, names) + require.NotEmpty(tb, blobs) + require.Equal(tb, len(names), len(blobs)) mwBody := &bytes.Buffer{} mw := multipart.NewWriter(mwBody) err := mw.WriteField("channel_id", channelId) - require.NoError(t, err) + require.NoError(tb, err) for i, blob := range blobs { ct := http.DetectContentType(blob) if len(clientIds) > i { err = mw.WriteField("client_ids", clientIds[i]) - require.NoError(t, err) + require.NoError(tb, err) } h := textproto.MIMEHeader{} @@ -185,18 +185,18 @@ func testUploadFilesMultipart( // If we error here, writing to mw, the deferred handler var part io.Writer part, err = mw.CreatePart(h) - require.NoError(t, err) + require.NoError(tb, err) _, err = io.Copy(part, bytes.NewReader(blob)) - require.NoError(t, err) + require.NoError(tb, err) } - require.NoError(t, mw.Close()) + require.NoError(tb, mw.Close()) url := "" if isBookmark { url += "?bookmark=true" } - fur, resp, err := testDoUploadFileRequest(t, c, url, mwBody.Bytes(), mw.FormDataContentType(), -1) + fur, resp, err := testDoUploadFileRequest(tb, c, url, mwBody.Bytes(), mw.FormDataContentType(), -1) if err != nil { return nil, resp, err } @@ -240,7 +240,7 @@ func TestUploadFiles(t *testing.T) { expectedImageHasPreview []bool expectedImageMiniPreview []bool setupConfig func(a *app.App) func(a *app.App) - checkResponse func(t testing.TB, resp *model.Response) + checkResponse func(tb testing.TB, resp *model.Response) uploadAsBookmark bool }{ // Upload a bunch of files, mixed images and non-images diff --git a/server/channels/api4/team_test.go b/server/channels/api4/team_test.go index 44f24b7a0b..4667b82643 100644 --- a/server/channels/api4/team_test.go +++ b/server/channels/api4/team_test.go @@ -1068,7 +1068,7 @@ func TestUpdateTeamPrivacy(t *testing.T) { name string team *model.Team privacy string - errChecker func(t testing.TB, resp *model.Response) + errChecker func(tb testing.TB, resp *model.Response) wantType string wantOpenInvite bool wantInviteIdChanged bool diff --git a/server/channels/app/oauth_test.go b/server/channels/app/oauth_test.go index ee5fc9bf4f..dd523002f1 100644 --- a/server/channels/app/oauth_test.go +++ b/server/channels/app/oauth_test.go @@ -84,8 +84,7 @@ func TestOAuthRevokeAccessToken(t *testing.T) { session.Roles = model.SystemUserRoleId th.App.SetSessionExpireInHours(session, 24) - var err *model.AppError - session, err = th.App.CreateSession(th.Context, session) + session, err := th.App.CreateSession(th.Context, session) require.Nil(t, err) err = th.App.RevokeAccessToken(th.Context, session.Token) require.NotNil(t, err, "Should have failed does not have an access token") @@ -104,9 +103,8 @@ func TestOAuthDeleteApp(t *testing.T) { a1.CallbackUrls = []string{"https://nowhere.com"} a1.Homepage = "https://nowhere.com" - var err *model.AppError - a1, err = th.App.CreateOAuthApp(a1) - require.Nil(t, err) + a1, appErr := th.App.CreateOAuthApp(a1) + require.Nil(t, appErr) session := &model.Session{} session.CreateAt = model.GetMillis() @@ -116,7 +114,7 @@ func TestOAuthDeleteApp(t *testing.T) { session.IsOAuth = true th.App.ch.srv.platform.SetSessionExpireInHours(session, 24) - session, appErr := th.App.CreateSession(th.Context, session) + session, appErr = th.App.CreateSession(th.Context, session) require.Nil(t, appErr) accessData := &model.AccessData{} @@ -126,14 +124,14 @@ func TestOAuthDeleteApp(t *testing.T) { accessData.ClientId = a1.Id accessData.ExpiresAt = session.ExpiresAt - _, nErr := th.App.Srv().Store().OAuth().SaveAccessData(accessData) - require.NoError(t, nErr) + _, err := th.App.Srv().Store().OAuth().SaveAccessData(accessData) + require.NoError(t, err) - err = th.App.DeleteOAuthApp(th.Context, a1.Id) - require.Nil(t, err) + appErr = th.App.DeleteOAuthApp(th.Context, a1.Id) + require.Nil(t, appErr) - _, err = th.App.GetSession(session.Token) - require.NotNil(t, err, "should not get session from cache or db") + _, appErr = th.App.GetSession(session.Token) + require.NotNil(t, appErr, "should not get session from cache or db") } func TestAuthorizeOAuthUser(t *testing.T) { @@ -166,12 +164,14 @@ func TestAuthorizeOAuthUser(t *testing.T) { } makeToken := func(th *TestHelper, cookie string) *model.Token { - token, _ := th.App.CreateOAuthStateToken(generateOAuthStateTokenExtra("", "", cookie)) + token, appErr := th.App.CreateOAuthStateToken(generateOAuthStateTokenExtra("", "", cookie)) + require.Nil(t, appErr) return token } makeRequest := func(cookie string) *http.Request { - request, _ := http.NewRequest(http.MethodGet, "https://mattermost.example.com", nil) + request, err := http.NewRequest(http.MethodGet, "https://mattermost.example.com", nil) + require.NoError(t, err) if cookie != "" { request.AddCookie(&http.Cookie{ @@ -615,8 +615,8 @@ func TestDeauthorizeOAuthApp(t *testing.T) { CallbackUrls: []string{"https://nowhere.com"}, } - oapp, err := th.App.CreateOAuthApp(oapp) - require.Nil(t, err) + oapp, appErr := th.App.CreateOAuthApp(oapp) + require.Nil(t, appErr) authRequest := &model.AuthorizeRequest{ ResponseType: model.ImplicitResponseType, @@ -626,8 +626,8 @@ func TestDeauthorizeOAuthApp(t *testing.T) { State: "123", } - redirectUrl, err := th.App.GetOAuthCodeRedirect(th.BasicUser.Id, authRequest) - assert.Nil(t, err) + redirectUrl, appErr := th.App.GetOAuthCodeRedirect(th.BasicUser.Id, authRequest) + assert.Nil(t, appErr) dErr := th.App.DeauthorizeOAuthAppForUser(th.Context, th.BasicUser.Id, oapp.Id) assert.Nil(t, dErr) @@ -638,8 +638,8 @@ func TestDeauthorizeOAuthApp(t *testing.T) { queryParams := uri.Query() code := queryParams.Get("code") - data, nErr := th.App.Srv().Store().OAuth().GetAuthData(code) - require.Equal(t, store.NewErrNotFound("AuthData", fmt.Sprintf("code=%s", code)), nErr) + data, err := th.App.Srv().Store().OAuth().GetAuthData(code) + require.Equal(t, store.NewErrNotFound("AuthData", fmt.Sprintf("code=%s", code)), err) assert.Nil(t, data) } @@ -657,8 +657,8 @@ func TestDeactivatedUserOAuthApp(t *testing.T) { CallbackUrls: []string{"https://nowhere.com"}, } - oapp, err := th.App.CreateOAuthApp(oapp) - require.Nil(t, err) + oapp, appErr := th.App.CreateOAuthApp(oapp) + require.Nil(t, appErr) authRequest := &model.AuthorizeRequest{ ResponseType: model.ImplicitResponseType, @@ -668,21 +668,21 @@ func TestDeactivatedUserOAuthApp(t *testing.T) { State: "123", } - redirectUrl, err := th.App.GetOAuthCodeRedirect(th.BasicUser.Id, authRequest) - assert.Nil(t, err) + redirectUrl, appErr := th.App.GetOAuthCodeRedirect(th.BasicUser.Id, authRequest) + assert.Nil(t, appErr) - uri, uErr := url.Parse(redirectUrl) - require.NoError(t, uErr) + uri, err := url.Parse(redirectUrl) + require.NoError(t, err) queryParams := uri.Query() code := queryParams.Get("code") - _, appErr := th.App.UpdateActive(th.Context, th.BasicUser, false) + _, appErr = th.App.UpdateActive(th.Context, th.BasicUser, false) require.Nil(t, appErr) - resp, accErr := th.App.GetOAuthAccessTokenForCodeFlow(th.Context, oapp.Id, model.AccessTokenGrantType, oapp.CallbackUrls[0], code, oapp.ClientSecret, "") + resp, appErr := th.App.GetOAuthAccessTokenForCodeFlow(th.Context, oapp.Id, model.AccessTokenGrantType, oapp.CallbackUrls[0], code, oapp.ClientSecret, "") assert.Nil(t, resp) - require.NotNil(t, accErr, "Should not get access token") - require.Equal(t, http.StatusBadRequest, accErr.StatusCode) - assert.Equal(t, "api.oauth.get_access_token.expired_code.app_error", accErr.Id) + require.NotNil(t, appErr, "Should not get access token") + require.Equal(t, http.StatusBadRequest, appErr.StatusCode) + assert.Equal(t, "api.oauth.get_access_token.expired_code.app_error", appErr.Id) } diff --git a/server/channels/jobs/batch_migration_worker_test.go b/server/channels/jobs/batch_migration_worker_test.go index c97894abc9..31b172e500 100644 --- a/server/channels/jobs/batch_migration_worker_test.go +++ b/server/channels/jobs/batch_migration_worker_test.go @@ -100,8 +100,7 @@ func TestBatchMigrationWorker(t *testing.T) { } t.Run("done after three batches", func(t *testing.T) { - th := Setup(t).InitBasic() - defer th.TearDown() + th := Setup(t).InitBasic(t) mockApp := &MockApp{} @@ -128,8 +127,7 @@ func TestBatchMigrationWorker(t *testing.T) { }) t.Run("clusters not in sync before first batch", func(t *testing.T) { - th := Setup(t).InitBasic() - defer th.TearDown() + th := Setup(t).InitBasic(t) mockApp := &MockApp{} mockApp.SetOutOfSync() @@ -155,8 +153,7 @@ func TestBatchMigrationWorker(t *testing.T) { }) t.Run("clusters not in sync after first batch", func(t *testing.T) { - th := Setup(t).InitBasic() - defer th.TearDown() + th := Setup(t).InitBasic(t) mockApp := &MockApp{} diff --git a/server/channels/jobs/batch_report_worker_test.go b/server/channels/jobs/batch_report_worker_test.go index d5ed657265..528809e210 100644 --- a/server/channels/jobs/batch_report_worker_test.go +++ b/server/channels/jobs/batch_report_worker_test.go @@ -83,8 +83,7 @@ func TestBatchReportWorker(t *testing.T) { } t.Run("should finish when the report is done, incrementing file count along the way", func(t *testing.T) { - th := Setup(t).InitBasic() - defer th.TearDown() + th := Setup(t).InitBasic(t) var worker model.Worker var job *model.Job @@ -114,8 +113,7 @@ func TestBatchReportWorker(t *testing.T) { }) t.Run("should fail job when get data throws an error", func(t *testing.T) { - th := Setup(t).InitBasic() - defer th.TearDown() + th := Setup(t).InitBasic(t) var worker model.Worker var job *model.Job diff --git a/server/channels/jobs/batch_worker_test.go b/server/channels/jobs/batch_worker_test.go index 36e61701b7..92131904da 100644 --- a/server/channels/jobs/batch_worker_test.go +++ b/server/channels/jobs/batch_worker_test.go @@ -18,7 +18,6 @@ import ( // cases of the batch worker. Use the -race flag while testing this. func TestBatchWorkerRace(t *testing.T) { th := Setup(t) - defer th.TearDown() worker := jobs.MakeBatchWorker(th.Server.Jobs, th.Server.Store(), 1*time.Second, func(rctx *request.Context, job *model.Job) bool { return false @@ -59,8 +58,7 @@ func TestBatchWorker(t *testing.T) { } t.Run("stop after first batch", func(t *testing.T) { - th := Setup(t).InitBasic() - defer th.TearDown() + th := Setup(t).InitBasic(t) var worker *jobs.BatchWorker worker, job := createBatchWorker(t, th, func(rctx *request.Context, job *model.Job) bool { @@ -86,8 +84,7 @@ func TestBatchWorker(t *testing.T) { }) t.Run("stop after second batch", func(t *testing.T) { - th := Setup(t).InitBasic() - defer th.TearDown() + th := Setup(t).InitBasic(t) var worker *jobs.BatchWorker worker, job := createBatchWorker(t, th, func(rctx *request.Context, job *model.Job) bool { @@ -113,8 +110,7 @@ func TestBatchWorker(t *testing.T) { }) t.Run("done after first batch", func(t *testing.T) { - th := Setup(t).InitBasic() - defer th.TearDown() + th := Setup(t).InitBasic(t) var worker *jobs.BatchWorker worker, job := createBatchWorker(t, th, func(rctx *request.Context, job *model.Job) bool { @@ -137,8 +133,7 @@ func TestBatchWorker(t *testing.T) { }) t.Run("done after three batches", func(t *testing.T) { - th := Setup(t).InitBasic() - defer th.TearDown() + th := Setup(t).InitBasic(t) var worker *jobs.BatchWorker worker, job := createBatchWorker(t, th, func(rctx *request.Context, job *model.Job) bool { diff --git a/server/channels/jobs/export_delete_test.go b/server/channels/jobs/export_delete_test.go index c300081b21..5350674f06 100644 --- a/server/channels/jobs/export_delete_test.go +++ b/server/channels/jobs/export_delete_test.go @@ -36,7 +36,6 @@ func TestExportDelete(t *testing.T) { } th := SetupWithUpdateCfg(t, updateConfig) - defer th.TearDown() // Create test files with different timestamps files := []string{ diff --git a/server/channels/jobs/helper_test.go b/server/channels/jobs/helper_test.go index 41fc4679c2..439d391fa9 100644 --- a/server/channels/jobs/helper_test.go +++ b/server/channels/jobs/helper_test.go @@ -36,15 +36,14 @@ type TestHelper struct { IncludeCacheLayer bool ConfigStore *config.Store - t testing.TB tempWorkspace string oldWatcherPollingInterval int } -func setupTestHelper(t testing.TB, dbStore store.Store, enterprise bool, includeCacheLayer bool, +func setupTestHelper(tb testing.TB, dbStore store.Store, enterprise bool, includeCacheLayer bool, updateCfg func(cfg *model.Config), options []app.Option) *TestHelper { tempWorkspace, err := os.MkdirTemp("", "jobstest") - require.NoError(t, err) + require.NoError(tb, err) configStore := config.NewTestMemoryStore() memoryConfig := configStore.Get() @@ -62,7 +61,7 @@ func setupTestHelper(t testing.TB, dbStore store.Store, enterprise bool, include } _, _, err = configStore.Set(memoryConfig) - require.NoError(t, err) + require.NoError(tb, err) buffer := &mlog.Buffer{} @@ -75,19 +74,19 @@ func setupTestHelper(t testing.TB, dbStore store.Store, enterprise bool, include } testLogger, err := mlog.NewLogger() - require.NoError(t, err) + require.NoError(tb, err) logCfg, err := config.MloggerConfigFromLoggerConfig(&memoryConfig.LogSettings, nil, config.GetLogFileLocation) - require.NoError(t, err) + require.NoError(tb, err) err = testLogger.ConfigureTargets(logCfg, nil) - require.NoError(t, err, "failed to configure test logger") + require.NoError(tb, err, "failed to configure test logger") err = mlog.AddWriterTarget(testLogger, buffer, true, mlog.StdAll...) - require.NoError(t, err, "failed to add writer target to test logger") + require.NoError(tb, err, "failed to add writer target to test logger") // lock logger config so server init cannot override it during testing. testLogger.LockConfiguration() options = append(options, app.SetLogger(testLogger)) s, err := app.NewServer(options...) - require.NoError(t, err) + require.NoError(tb, err) th := &TestHelper{ App: app.New(app.ServerConnector(s.Channels())), @@ -97,19 +96,47 @@ func setupTestHelper(t testing.TB, dbStore store.Store, enterprise bool, include TestLogger: testLogger, IncludeCacheLayer: includeCacheLayer, ConfigStore: configStore, - t: t, tempWorkspace: tempWorkspace, } prevListenAddress := *th.App.Config().ServiceSettings.ListenAddress th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = "localhost:0" }) err = th.Server.Start() - require.NoError(t, err) + require.NoError(tb, err) th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = prevListenAddress }) th.App.Srv().Store().MarkSystemRanUnitTests() + tb.Cleanup(func() { + if th.IncludeCacheLayer { + // Clean all the caches + appErr := th.App.Srv().InvalidateAllCaches() + require.Nil(tb, appErr) + } + done := make(chan bool) + go func() { + th.Server.Shutdown() + close(done) + }() + + select { + case <-done: + case <-time.After(30 * time.Second): + // panic instead of fatal to terminate all tests in this package, otherwise the + // still running App could spuriously fail subsequent tests. + panic("failed to shutdown App within 30 seconds") + } + + if th.tempWorkspace != "" { + os.RemoveAll(th.tempWorkspace) + } + + if th.oldWatcherPollingInterval != 0 { + jobs.DefaultWatcherPollingInterval = th.oldWatcherPollingInterval + } + }) + return th } @@ -118,6 +145,7 @@ func Setup(tb testing.TB, options ...app.Option) *TestHelper { } func SetupWithUpdateCfg(tb testing.TB, updateCfg func(cfg *model.Config), options ...app.Option) *TestHelper { + tb.Helper() if testing.Short() { tb.SkipNow() } @@ -135,27 +163,31 @@ func SetupWithUpdateCfg(tb testing.TB, updateCfg func(cfg *model.Config), option return th } -func (th *TestHelper) InitBasic() *TestHelper { - th.SystemAdminUser = th.CreateUser() +func (th *TestHelper) InitBasic(tb testing.TB) *TestHelper { + tb.Helper() + + th.SystemAdminUser = th.CreateUser(tb) _, appErr := th.App.UpdateUserRoles(th.Context, th.SystemAdminUser.Id, model.SystemUserRoleId+" "+model.SystemAdminRoleId, false) - require.Nil(th.t, appErr) + require.Nil(tb, appErr) th.SystemAdminUser, appErr = th.App.GetUser(th.SystemAdminUser.Id) - require.Nil(th.t, appErr) + require.Nil(tb, appErr) - th.BasicUser = th.CreateUser() + th.BasicUser = th.CreateUser(tb) th.BasicUser, appErr = th.App.GetUser(th.BasicUser.Id) - require.Nil(th.t, appErr) + require.Nil(tb, appErr) - th.BasicUser2 = th.CreateUser() + th.BasicUser2 = th.CreateUser(tb) th.BasicUser2, appErr = th.App.GetUser(th.BasicUser2.Id) - require.Nil(th.t, appErr) + require.Nil(tb, appErr) - th.BasicTeam = th.CreateTeam() + th.BasicTeam = th.CreateTeam(tb) return th } -func (th *TestHelper) CreateTeam() *model.Team { +func (th *TestHelper) CreateTeam(tb testing.TB) *model.Team { + tb.Helper() + id := model.NewId() team := &model.Team{ DisplayName: "dn_" + id, @@ -165,15 +197,17 @@ func (th *TestHelper) CreateTeam() *model.Team { } team, err := th.App.CreateTeam(th.Context, team) - require.Nil(th.t, err) + require.Nil(tb, err) return team } -func (th *TestHelper) CreateUser() *model.User { - return th.CreateUserOrGuest(false) +func (th *TestHelper) CreateUser(tb testing.TB) *model.User { + tb.Helper() + return th.CreateUserOrGuest(tb, false) } -func (th *TestHelper) CreateUserOrGuest(guest bool) *model.User { +func (th *TestHelper) CreateUserOrGuest(tb testing.TB, guest bool) *model.User { + tb.Helper() id := model.NewId() user := &model.User{ @@ -190,45 +224,12 @@ func (th *TestHelper) CreateUserOrGuest(guest bool) *model.User { } else { user, err = th.App.CreateUser(th.Context, user) } - require.Nil(th.t, err) + require.Nil(tb, err) return user } -func (th *TestHelper) ShutdownApp() { - done := make(chan bool) - go func() { - th.Server.Shutdown() - close(done) - }() - - select { - case <-done: - case <-time.After(30 * time.Second): - // panic instead of fatal to terminate all tests in this package, otherwise the - // still running App could spuriously fail subsequent tests. - panic("failed to shutdown App within 30 seconds") - } -} - -func (th *TestHelper) TearDown() { - if th.IncludeCacheLayer { - // Clean all the caches - appErr := th.App.Srv().InvalidateAllCaches() - require.Nil(th.t, appErr) - } - th.ShutdownApp() - - if th.tempWorkspace != "" { - os.RemoveAll(th.tempWorkspace) - } - - if th.oldWatcherPollingInterval != 0 { - jobs.DefaultWatcherPollingInterval = th.oldWatcherPollingInterval - } -} - -func (th *TestHelper) SetupBatchWorker(t *testing.T, worker *jobs.BatchWorker) *model.Job { - t.Helper() +func (th *TestHelper) SetupBatchWorker(tb testing.TB, worker *jobs.BatchWorker) *model.Job { + tb.Helper() jobId := model.NewId() th.Server.Jobs.RegisterJobType(jobId, worker, nil) @@ -236,7 +237,7 @@ func (th *TestHelper) SetupBatchWorker(t *testing.T, worker *jobs.BatchWorker) * jobData := make(model.StringMap) jobData["batch_number"] = "1" job, appErr := th.Server.Jobs.CreateJob(th.Context, jobId, jobData) - require.Nil(t, appErr) + require.Nil(tb, appErr) done := make(chan bool) go func() { @@ -245,8 +246,8 @@ func (th *TestHelper) SetupBatchWorker(t *testing.T, worker *jobs.BatchWorker) * }() // When ending the test, ensure we wait for the worker to finish. - t.Cleanup(func() { - waitDone(t, done, "worker did not stop running") + tb.Cleanup(func() { + waitDone(tb, done, "worker did not stop running") }) // Give the worker time to start running @@ -255,36 +256,36 @@ func (th *TestHelper) SetupBatchWorker(t *testing.T, worker *jobs.BatchWorker) * return job } -func (th *TestHelper) WaitForJobStatus(t *testing.T, job *model.Job, status string) { - t.Helper() +func (th *TestHelper) WaitForJobStatus(tb testing.TB, job *model.Job, status string) { + tb.Helper() - require.Eventuallyf(t, func() bool { + require.Eventuallyf(tb, func() bool { actualJob, appErr := th.Server.Jobs.GetJob(th.Context, job.Id) - require.Nil(t, appErr) - require.Equal(t, job.Id, actualJob.Id) + require.Nil(tb, appErr) + require.Equal(tb, job.Id, actualJob.Id) return actualJob.Status == status }, 5*time.Second, 250*time.Millisecond, "job never transitioned to %s", status) } -func (th *TestHelper) WaitForBatchNumber(t *testing.T, job *model.Job, batchNumber int) { - t.Helper() +func (th *TestHelper) WaitForBatchNumber(tb testing.TB, job *model.Job, batchNumber int) { + tb.Helper() - require.Eventuallyf(t, func() bool { + require.Eventuallyf(tb, func() bool { actualJob, appErr := th.Server.Jobs.GetJob(th.Context, job.Id) - require.Nil(t, appErr) - require.Equal(t, job.Id, actualJob.Id) + require.Nil(tb, appErr) + require.Equal(tb, job.Id, actualJob.Id) finalBatchNumber, err := strconv.Atoi(actualJob.Data["batch_number"]) - require.NoError(t, err) + require.NoError(tb, err) return finalBatchNumber == batchNumber }, 5*time.Second, 250*time.Millisecond, "job did not stop at batch %d", batchNumber) } -func waitDone(t *testing.T, done chan bool, msg string) { - t.Helper() +func waitDone(tb testing.TB, done chan bool, msg string) { + tb.Helper() - require.Eventually(t, func() bool { + require.Eventually(tb, func() bool { select { case <-done: return true @@ -294,32 +295,34 @@ func waitDone(t *testing.T, done chan bool, msg string) { }, 5*time.Second, 100*time.Millisecond, msg) } -func (th *TestHelper) SetupWorkers(t *testing.T) { +func (th *TestHelper) SetupWorkers(tb testing.TB) { + tb.Helper() + err := th.App.Srv().Jobs.StartWorkers() - require.NoError(t, err) + require.NoError(tb, err) } -func (th *TestHelper) RunJob(t *testing.T, jobType string, jobData map[string]string) *model.Job { - t.Helper() +func (th *TestHelper) RunJob(tb testing.TB, jobType string, jobData map[string]string) *model.Job { + tb.Helper() job, appErr := th.Server.Jobs.CreateJob(th.Context, jobType, jobData) - require.Nil(t, appErr) + require.Nil(tb, appErr) // poll until completion - th.checkJobStatus(t, job.Id, model.JobStatusSuccess) + th.checkJobStatus(tb, job.Id, model.JobStatusSuccess) job, appErr = th.Server.Jobs.GetJob(th.Context, job.Id) - require.Nil(t, appErr) + require.Nil(tb, appErr) return job } -func (th *TestHelper) checkJobStatus(t *testing.T, jobId string, status string) { - t.Helper() +func (th *TestHelper) checkJobStatus(tb testing.TB, jobId string, status string) { + tb.Helper() - require.Eventuallyf(t, func() bool { + require.Eventuallyf(tb, func() bool { // it's ok if there's an error, it might take awhile for the job to finish. job, appErr := th.Server.Jobs.GetJob(th.Context, jobId) - assert.Nil(th.t, appErr) + assert.Nil(tb, appErr) if jobId == job.Id { return job.Status == status } diff --git a/server/channels/utils/emoji.go b/server/channels/utils/emoji.go index 82ef927bf8..9e70b89fd6 100644 --- a/server/channels/utils/emoji.go +++ b/server/channels/utils/emoji.go @@ -15,16 +15,16 @@ import ( "github.com/stretchr/testify/require" ) -func CreateTestGif(t testing.TB, width int, height int) []byte { +func CreateTestGif(tb testing.TB, width int, height int) []byte { var buffer bytes.Buffer err := gif.Encode(&buffer, image.NewRGBA(image.Rect(0, 0, width, height)), nil) - require.NoErrorf(t, err, "failed to create gif: %v", err) + require.NoErrorf(tb, err, "failed to create gif: %v", err) return buffer.Bytes() } -func CreateTestAnimatedGif(t *testing.T, width int, height int, frames int) []byte { +func CreateTestAnimatedGif(tb testing.TB, width int, height int, frames int) []byte { var buffer bytes.Buffer img := gif.GIF{ @@ -36,25 +36,25 @@ func CreateTestAnimatedGif(t *testing.T, width int, height int, frames int) []by img.Delay[i] = 0 } err := gif.EncodeAll(&buffer, &img) - require.NoErrorf(t, err, "failed to create animated gif: %v", err) + require.NoErrorf(tb, err, "failed to create animated gif: %v", err) return buffer.Bytes() } -func CreateTestJpeg(t *testing.T, width int, height int) []byte { +func CreateTestJpeg(tb testing.TB, width int, height int) []byte { var buffer bytes.Buffer err := jpeg.Encode(&buffer, image.NewRGBA(image.Rect(0, 0, width, height)), nil) - require.NoErrorf(t, err, "failed to create jpeg: %v", err) + require.NoErrorf(tb, err, "failed to create jpeg: %v", err) return buffer.Bytes() } -func CreateTestPng(t testing.TB, width int, height int) []byte { +func CreateTestPng(tb testing.TB, width int, height int) []byte { var buffer bytes.Buffer err := png.Encode(&buffer, image.NewRGBA(image.Rect(0, 0, width, height))) - require.NoErrorf(t, err, "failed to create png: %v", err) + require.NoErrorf(tb, err, "failed to create png: %v", err) return buffer.Bytes() } diff --git a/server/channels/web/context_test.go b/server/channels/web/context_test.go index 79f8f1d8ea..d103e77339 100644 --- a/server/channels/web/context_test.go +++ b/server/channels/web/context_test.go @@ -36,7 +36,6 @@ func TestRequireHookId(t *testing.T) { func TestCloudKeyRequired(t *testing.T) { th := SetupWithStoreMock(t) - defer th.TearDown() th.App.Srv().SetLicense(model.NewTestLicense("cloud")) @@ -52,7 +51,6 @@ func TestCloudKeyRequired(t *testing.T) { func TestMfaRequired(t *testing.T) { th := SetupWithStoreMock(t) - defer th.TearDown() mockStore := th.App.Srv().Store().(*mocks.Store) mockUserStore := mocks.UserStore{} diff --git a/server/channels/web/handlers_test.go b/server/channels/web/handlers_test.go index 97a8722b85..eadcae5259 100644 --- a/server/channels/web/handlers_test.go +++ b/server/channels/web/handlers_test.go @@ -27,7 +27,6 @@ func handlerForHTTPErrors(c *Context, w http.ResponseWriter, r *http.Request) { func TestHandlerServeHTTPErrors(t *testing.T) { th := SetupWithStoreMock(t) - defer th.TearDown() web := New(th.Server) handler := web.NewHandler(handlerForHTTPErrors) @@ -67,7 +66,6 @@ func handlerForServeDefaultSecurityHeaders(c *Context, w http.ResponseWriter, r func TestHandlerServeDefaultSecurityHeaders(t *testing.T) { th := SetupWithStoreMock(t) - defer th.TearDown() web := New(th.Server) handler := web.NewHandler(handlerForServeDefaultSecurityHeaders) @@ -105,7 +103,6 @@ func handlerForHTTPSecureTransport(c *Context, w http.ResponseWriter, r *http.Re func TestHandlerServeHTTPSecureTransport(t *testing.T) { th := SetupWithStoreMock(t) - defer th.TearDown() mockStore := th.App.Srv().Store().(*mocks.Store) mockUserStore := mocks.UserStore{} @@ -163,8 +160,7 @@ func handlerForCSRFToken(c *Context, w http.ResponseWriter, r *http.Request) { } func TestHandlerServeCSRFToken(t *testing.T) { - th := Setup(t).InitBasic() - defer th.TearDown() + th := Setup(t).InitBasic(t) session := &model.Session{ UserId: th.BasicUser.Id, @@ -175,9 +171,7 @@ func TestHandlerServeCSRFToken(t *testing.T) { session.GenerateCSRF() th.App.SetSessionExpireInHours(session, 24) session, err := th.App.CreateSession(th.Context, session) - if err != nil { - t.Errorf("Expected nil, got %s", err) - } + require.Nil(t, err) web := New(th.Server) @@ -304,7 +298,6 @@ func handlerForCSPHeader(c *Context, w http.ResponseWriter, r *http.Request) { func TestHandlerServeCSPHeader(t *testing.T) { t.Run("non-static", func(t *testing.T) { th := SetupWithStoreMock(t) - defer th.TearDown() web := New(th.Server) @@ -326,7 +319,6 @@ func TestHandlerServeCSPHeader(t *testing.T) { t.Run("static, without subpath", func(t *testing.T) { th := SetupWithStoreMock(t) - defer th.TearDown() web := New(th.Server) @@ -348,7 +340,6 @@ func TestHandlerServeCSPHeader(t *testing.T) { t.Run("static, with subpath and frame ancestors", func(t *testing.T) { th := SetupWithStoreMock(t) - defer th.TearDown() mockStore := th.App.Srv().Store().(*mocks.Store) mockUserStore := mocks.UserStore{} @@ -407,7 +398,6 @@ func TestHandlerServeCSPHeader(t *testing.T) { t.Run("dev mode", func(t *testing.T) { th := Setup(t) - defer th.TearDown() oldBuildNumber := model.BuildNumber model.BuildNumber = "dev" @@ -437,7 +427,6 @@ func TestHandlerServeCSPHeader(t *testing.T) { func TestGenerateDevCSP(t *testing.T) { t.Run("dev mode", func(t *testing.T) { th := Setup(t) - defer th.TearDown() oldBuildNumber := model.BuildNumber model.BuildNumber = "dev" @@ -457,7 +446,6 @@ func TestGenerateDevCSP(t *testing.T) { t.Run("allowed dev flags", func(t *testing.T) { th := Setup(t) - defer th.TearDown() oldBuildNumber := model.BuildNumber model.BuildNumber = "0" @@ -482,7 +470,6 @@ func TestGenerateDevCSP(t *testing.T) { t.Run("partial dev flags", func(t *testing.T) { th := Setup(t) - defer th.TearDown() oldBuildNumber := model.BuildNumber model.BuildNumber = "0" @@ -507,7 +494,6 @@ func TestGenerateDevCSP(t *testing.T) { t.Run("unknown dev flags", func(t *testing.T) { th := Setup(t) - defer th.TearDown() oldBuildNumber := model.BuildNumber model.BuildNumber = "0" @@ -532,7 +518,6 @@ func TestGenerateDevCSP(t *testing.T) { t.Run("empty dev flags", func(t *testing.T) { th := Setup(t) - defer th.TearDown() th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.DeveloperFlags = "" @@ -567,7 +552,6 @@ func TestHandlerServeInvalidToken(t *testing.T) { for _, tc := range testCases { t.Run(tc.Description, func(t *testing.T) { th := Setup(t) - defer th.TearDown() th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.SiteURL = tc.SiteURL @@ -604,7 +588,6 @@ func TestHandlerServeInvalidToken(t *testing.T) { func TestCheckCSRFToken(t *testing.T) { t.Run("should allow a POST request with a valid CSRF token header", func(t *testing.T) { th := SetupWithStoreMock(t) - defer th.TearDown() h := &Handler{ RequireSession: true, @@ -635,7 +618,6 @@ func TestCheckCSRFToken(t *testing.T) { t.Run("should allow a POST request with an X-Requested-With header", func(t *testing.T) { th := SetupWithStoreMock(t) - defer th.TearDown() h := &Handler{ RequireSession: true, @@ -667,7 +649,6 @@ func TestCheckCSRFToken(t *testing.T) { t.Run("should not allow a POST request with an X-Requested-With header with strict CSRF enforcement enabled", func(t *testing.T) { th := SetupWithStoreMock(t) - defer th.TearDown() mockStore := th.App.Srv().Store().(*mocks.Store) mockUserStore := mocks.UserStore{} @@ -718,7 +699,6 @@ func TestCheckCSRFToken(t *testing.T) { t.Run("should not allow a POST request without either header", func(t *testing.T) { th := SetupWithStoreMock(t) - defer th.TearDown() h := &Handler{ RequireSession: true, @@ -748,7 +728,6 @@ func TestCheckCSRFToken(t *testing.T) { t.Run("should not check GET requests", func(t *testing.T) { th := SetupWithStoreMock(t) - defer th.TearDown() h := &Handler{ RequireSession: true, @@ -778,7 +757,6 @@ func TestCheckCSRFToken(t *testing.T) { t.Run("should not check a request passing the auth token in a header", func(t *testing.T) { th := SetupWithStoreMock(t) - defer th.TearDown() h := &Handler{ RequireSession: true, @@ -808,7 +786,6 @@ func TestCheckCSRFToken(t *testing.T) { t.Run("should not check a request passing a nil session", func(t *testing.T) { th := SetupWithStoreMock(t) - defer th.TearDown() h := &Handler{ RequireSession: false, @@ -834,7 +811,6 @@ func TestCheckCSRFToken(t *testing.T) { t.Run("should check requests for handlers that don't require a session but have one", func(t *testing.T) { th := SetupWithStoreMock(t) - defer th.TearDown() h := &Handler{ RequireSession: false, @@ -939,7 +915,6 @@ func noOpHandler(_ *Context, _ http.ResponseWriter, _ *http.Request) { func TestHandlerServeHTTPBasicSecurityChecks(t *testing.T) { t.Run("Should not cause 414 error if url is smaller than configured limit", func(t *testing.T) { th := SetupWithStoreMock(t) - defer th.TearDown() web := New(th.Server) handler := web.NewHandler(noOpHandler) @@ -953,7 +928,6 @@ func TestHandlerServeHTTPBasicSecurityChecks(t *testing.T) { t.Run("Should cause 414 error if url is longer than configured limit", func(t *testing.T) { th := SetupWithStoreMock(t) - defer th.TearDown() mockStore := th.App.Srv().Store().(*mocks.Store) mockUserStore := mocks.UserStore{} @@ -985,7 +959,6 @@ func TestHandlerServeHTTPBasicSecurityChecks(t *testing.T) { t.Run("414 error should include query params in computing URL length", func(t *testing.T) { th := SetupWithStoreMock(t) - defer th.TearDown() mockStore := th.App.Srv().Store().(*mocks.Store) mockUserStore := mocks.UserStore{} @@ -1034,7 +1007,6 @@ func TestHandlerServeHTTPRequestPayloadLimit(t *testing.T) { t.Run("should allow payload smaller than set limit", func(t *testing.T) { th := SetupWithStoreMock(t) - defer th.TearDown() web := New(th.Server) handler := web.NewHandler(jsonReaderHandler) @@ -1049,7 +1021,6 @@ func TestHandlerServeHTTPRequestPayloadLimit(t *testing.T) { t.Run("Should error out when request body is larger than set limit", func(t *testing.T) { th := SetupWithStoreMock(t) - defer th.TearDown() mockStore := th.App.Srv().Store().(*mocks.Store) mockUserStore := mocks.UserStore{} diff --git a/server/channels/web/oauth_test.go b/server/channels/web/oauth_test.go index 2e833742c0..20f0d056d9 100644 --- a/server/channels/web/oauth_test.go +++ b/server/channels/web/oauth_test.go @@ -24,8 +24,7 @@ import ( ) func TestOAuthComplete_AccessDenied(t *testing.T) { - th := Setup(t).InitBasic() - defer th.TearDown() + th := Setup(t).InitBasic(t) c := &Context{ App: th.App, @@ -35,7 +34,8 @@ func TestOAuthComplete_AccessDenied(t *testing.T) { AppContext: request.EmptyContext(th.TestLogger), } responseWriter := httptest.NewRecorder() - request, _ := http.NewRequest(http.MethodGet, th.App.GetSiteURL()+"/signup/TestService/complete?error=access_denied", nil) + request, err := http.NewRequest(http.MethodGet, th.App.GetSiteURL()+"/signup/TestService/complete?error=access_denied", nil) + require.NoError(t, err) completeOAuth(c, responseWriter, request) @@ -43,15 +43,15 @@ func TestOAuthComplete_AccessDenied(t *testing.T) { assert.Equal(t, http.StatusTemporaryRedirect, response.StatusCode) - location, _ := url.Parse(response.Header.Get("Location")) + location, err := url.Parse(response.Header.Get("Location")) + require.NoError(t, err) assert.Equal(t, "oauth_access_denied", location.Query().Get("type")) assert.Equal(t, "TestService", location.Query().Get("service")) } func TestAuthorizeOAuthApp(t *testing.T) { - th := Setup(t).InitBasic() - th.Login(apiClient, th.SystemAdminUser) - defer th.TearDown() + th := Setup(t).InitBasic(t) + th.Login(t, apiClient, th.SystemAdminUser) enableOAuth := *th.App.Config().ServiceSettings.EnableOAuthServiceProvider defer func() { @@ -85,7 +85,8 @@ func TestAuthorizeOAuthApp(t *testing.T) { require.NotEmpty(t, ruri, "redirect url should be set") - ru, _ := url.Parse(ruri) + ru, err := url.Parse(ruri) + require.NoError(t, err) require.NotNil(t, ru, "redirect url unparseable") require.NotEmpty(t, ru.Query().Get("code"), "authorization code not returned") require.Equal(t, ru.Query().Get("state"), authRequest.State, "returned state doesn't match") @@ -96,7 +97,8 @@ func TestAuthorizeOAuthApp(t *testing.T) { require.NoError(t, err) require.False(t, ruri == "", "redirect url should be set") - ru, _ = url.Parse(ruri) + ru, err = url.Parse(ruri) + require.NoError(t, err) require.NotNil(t, ru, "redirect url unparseable") values, err := url.ParseQuery(ru.Fragment) require.NoError(t, err) @@ -159,7 +161,8 @@ func TestAuthorizeOAuthApp(t *testing.T) { } uriResponse, _, err := apiClient.AuthorizeOAuthApp(context.Background(), authRequest) require.NoError(t, err) - ru, _ = url.Parse(uriResponse) + ru, err = url.Parse(uriResponse) + require.NoError(t, err) require.NotEmpty(t, uriResponse, "redirect url should be set") require.NotNil(t, ru, "redirect url unparseable") // require no query parameter to have "?" @@ -173,9 +176,8 @@ func TestAuthorizeOAuthApp(t *testing.T) { } func TestDeauthorizeOAuthApp(t *testing.T) { - th := Setup(t).InitBasic() - th.Login(apiClient, th.SystemAdminUser) - defer th.TearDown() + th := Setup(t).InitBasic(t) + th.Login(t, apiClient, th.SystemAdminUser) enableOAuth := th.App.Config().ServiceSettings.EnableOAuthServiceProvider defer func() { @@ -226,9 +228,8 @@ func TestOAuthAccessToken(t *testing.T) { t.SkipNow() } - th := Setup(t).InitBasic() - th.Login(apiClient, th.SystemAdminUser) - defer th.TearDown() + th := Setup(t).InitBasic(t) + th.Login(t, apiClient, th.SystemAdminUser) enableOAuth := th.App.Config().ServiceSettings.EnableOAuthServiceProvider defer func() { @@ -270,14 +271,13 @@ func TestOAuthAccessToken(t *testing.T) { redirect, _, err := apiClient.AuthorizeOAuthApp(context.Background(), authRequest) require.NoError(t, err) - rurl, _ := url.Parse(redirect) - - apiClient.Logout(context.Background()) + rurl, err := url.Parse(redirect) + require.NoError(t, err) data = url.Values{"grant_type": []string{"junk"}, "client_id": []string{oauthApp.Id}, "client_secret": []string{oauthApp.ClientSecret}, "code": []string{rurl.Query().Get("code")}, "redirect_uri": []string{oauthApp.CallbackUrls[0]}} - - _, _, err = apiClient.GetOAuthAccessToken(context.Background(), data) + _, resp, err := apiClient.GetOAuthAccessToken(context.Background(), data) require.Error(t, err, "should have failed - bad grant type") + CheckBadRequestStatus(t, resp) data.Set("grant_type", model.AccessTokenGrantType) data.Set("client_id", "") @@ -395,8 +395,8 @@ func TestOAuthAccessToken(t *testing.T) { } func TestMobileLoginWithOAuth(t *testing.T) { - th := Setup(t).InitBasic() - defer th.TearDown() + th := Setup(t).InitBasic(t) + c := &Context{ App: th.App, AppContext: th.Context, @@ -417,7 +417,8 @@ func TestMobileLoginWithOAuth(t *testing.T) { t.Run("Should redirect to the SSO login page when valid URL Scheme is passed as redirect_to parameter", func(t *testing.T) { responseWriter := httptest.NewRecorder() - request, _ := http.NewRequest(http.MethodGet, th.App.GetSiteURL()+"/oauth/gitlab/mobile_login?redirect_to="+url.QueryEscape("mmauth://"), nil) + request, err := http.NewRequest(http.MethodGet, th.App.GetSiteURL()+"/oauth/gitlab/mobile_login?redirect_to="+url.QueryEscape("mmauth://"), nil) + require.NoError(t, err) mobileLoginWithOAuth(c, responseWriter, request) assert.Equal(t, responseWriter.Code, 302) assert.NotContains(t, responseWriter.Body.String(), siteURL) @@ -426,7 +427,8 @@ func TestMobileLoginWithOAuth(t *testing.T) { t.Run("Should include SiteURL in the output when invalid URL Scheme is passed", func(t *testing.T) { einterfaces.RegisterOAuthProvider(model.ServiceGitlab, provider) responseWriter := httptest.NewRecorder() - request, _ := http.NewRequest(http.MethodGet, th.App.GetSiteURL()+"/oauth/gitlab/mobile_login?redirect_to="+url.QueryEscape("randomScheme://"), nil) + request, err := http.NewRequest(http.MethodGet, th.App.GetSiteURL()+"/oauth/gitlab/mobile_login?redirect_to="+url.QueryEscape("randomScheme://"), nil) + require.NoError(t, err) mobileLoginWithOAuth(c, responseWriter, request) body := responseWriter.Body.String() assert.NotContains(t, body, "randomScheme://") @@ -435,7 +437,8 @@ func TestMobileLoginWithOAuth(t *testing.T) { t.Run("Should not include the redirect URL consisting of javascript protocol", func(t *testing.T) { responseWriter := httptest.NewRecorder() - request, _ := http.NewRequest(http.MethodGet, th.App.GetSiteURL()+"/oauth/gitlab/mobile_login?redirect_to="+url.QueryEscape("javascript:alert('hello')"), nil) + request, err := http.NewRequest(http.MethodGet, th.App.GetSiteURL()+"/oauth/gitlab/mobile_login?redirect_to="+url.QueryEscape("javascript:alert('hello')"), nil) + require.NoError(t, err) mobileLoginWithOAuth(c, responseWriter, request) body := responseWriter.Body.String() assert.NotContains(t, body, "javascript:alert('hello')") @@ -444,7 +447,8 @@ func TestMobileLoginWithOAuth(t *testing.T) { t.Run("Should not include the redirect URL consisting of javascript protocol in mixed case", func(t *testing.T) { responseWriter := httptest.NewRecorder() - request, _ := http.NewRequest(http.MethodGet, th.App.GetSiteURL()+"/oauth/gitlab/mobile_login?redirect_to="+url.QueryEscape("JaVasCript:alert('hello')"), nil) + request, err := http.NewRequest(http.MethodGet, th.App.GetSiteURL()+"/oauth/gitlab/mobile_login?redirect_to="+url.QueryEscape("JaVasCript:alert('hello')"), nil) + require.NoError(t, err) mobileLoginWithOAuth(c, responseWriter, request) body := responseWriter.Body.String() assert.NotContains(t, body, "JaVasCript:alert('hello')") @@ -457,9 +461,8 @@ func TestOAuthComplete(t *testing.T) { t.SkipNow() } - th := Setup(t).InitBasic() - th.Login(apiClient, th.SystemAdminUser) - defer th.TearDown() + th := Setup(t).InitBasic(t) + th.Login(t, apiClient, th.SystemAdminUser) gitLabSettingsEnable := th.App.Config().GitLabSettings.Enable gitLabSettingsAuthEndpoint := th.App.Config().GitLabSettings.AuthEndpoint @@ -548,7 +551,8 @@ func TestOAuthComplete(t *testing.T) { redirect, _, err := apiClient.AuthorizeOAuthApp(context.Background(), authRequest) require.NoError(t, err) - rurl, _ := url.Parse(redirect) + rurl, err := url.Parse(redirect) + require.NoError(t, err) code := rurl.Query().Get("code") stateProps["action"] = model.OAuthActionEmailToSSO @@ -613,8 +617,8 @@ func TestOAuthComplete(t *testing.T) { } func TestOAuthComplete_ErrorMessages(t *testing.T) { - th := Setup(t).InitBasic() - defer th.TearDown() + th := Setup(t).InitBasic(t) + c := &Context{ App: th.App, AppContext: th.Context, @@ -632,7 +636,8 @@ func TestOAuthComplete_ErrorMessages(t *testing.T) { responseWriter := httptest.NewRecorder() // Renders for web & mobile app with webview - request, _ := http.NewRequest(http.MethodGet, th.App.GetSiteURL()+"/signup/gitlab/complete?code=1234", nil) + request, err := http.NewRequest(http.MethodGet, th.App.GetSiteURL()+"/signup/gitlab/complete?code=1234", nil) + require.NoError(t, err) completeOAuth(c, responseWriter, request) assert.Contains(t, responseWriter.Body.String(), "") @@ -642,14 +647,18 @@ func TestOAuthComplete_ErrorMessages(t *testing.T) { stateProps["action"] = model.OAuthActionMobile stateProps["redirect_to"] = th.App.Config().NativeAppSettings.AppCustomURLSchemes[0] state := base64.StdEncoding.EncodeToString([]byte(model.MapToJSON(stateProps))) - request2, _ := http.NewRequest(http.MethodGet, th.App.GetSiteURL()+"/signup/gitlab/complete?code=1234&state="+url.QueryEscape(state), nil) + request2, err := http.NewRequest(http.MethodGet, th.App.GetSiteURL()+"/signup/gitlab/complete?code=1234&state="+url.QueryEscape(state), nil) + require.NoError(t, err) completeOAuth(c, responseWriter, request2) assert.Contains(t, responseWriter.Body.String(), "") } func HTTPGet(url string, httpClient *http.Client, authToken string, followRedirect bool) (*http.Response, error) { - rq, _ := http.NewRequest("GET", url, nil) + rq, err := http.NewRequest("GET", url, nil) + if err != nil { + return nil, err + } rq.Close = true if authToken != "" { @@ -678,7 +687,7 @@ func HTTPGet(url string, httpClient *http.Client, authToken string, followRedire func closeBody(r *http.Response) { if r != nil && r.Body != nil { - io.ReadAll(r.Body) + _, _ = io.ReadAll(r.Body) // Discard and ignore errors - just draining the body r.Body.Close() } } @@ -739,13 +748,16 @@ func CheckBadRequestStatus(t *testing.T, resp *model.Response) { checkHTTPStatus(t, resp, http.StatusBadRequest) } -func (th *TestHelper) Login(client *model.Client4, user *model.User) { +func (th *TestHelper) Login(tb testing.TB, client *model.Client4, user *model.User) { + tb.Helper() + session := &model.Session{ UserId: user.Id, Roles: user.GetRawRoles(), IsOAuth: false, } - session, _ = th.App.CreateSession(th.Context, session) + session, appErr := th.App.CreateSession(th.Context, session) + require.Nil(tb, appErr) client.AuthToken = session.Token client.AuthType = model.HeaderBearer } diff --git a/server/channels/web/web_test.go b/server/channels/web/web_test.go index 261b1b49e9..410dd15b59 100644 --- a/server/channels/web/web_test.go +++ b/server/channels/web/web_test.go @@ -89,27 +89,24 @@ func setupTestHelper(tb testing.TB, includeCacheLayer bool, options []app.Option options = append(options, app.StoreOverride(mainHelper.Store)) } - testLogger, _ := mlog.NewLogger() - logCfg, _ := config.MloggerConfigFromLoggerConfig(&newConfig.LogSettings, nil, config.GetLogFileLocation) - if errCfg := testLogger.ConfigureTargets(logCfg, nil); errCfg != nil { - panic("failed to configure test logger: " + errCfg.Error()) - } + testLogger, err := mlog.NewLogger() + require.NoError(tb, err) + logCfg, err := config.MloggerConfigFromLoggerConfig(&newConfig.LogSettings, nil, config.GetLogFileLocation) + require.NoError(tb, err) + err = testLogger.ConfigureTargets(logCfg, nil) + require.NoError(tb, err, "failed to configure test logger") // lock logger config so server init cannot override it during testing. testLogger.LockConfiguration() options = append(options, app.SetLogger(testLogger)) s, err := app.NewServer(options...) - if err != nil { - panic(err) - } + require.NoError(tb, err) a := app.New(app.ServerConnector(s.Channels())) prevListenAddress := *s.Config().ServiceSettings.ListenAddress a.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = "localhost:0" }) - serverErr := s.Start() - if serverErr != nil { - panic(serverErr) - } + err = s.Start() + require.NoError(tb, err) a.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.ListenAddress = prevListenAddress }) // Disable strict password requirements for test @@ -140,6 +137,15 @@ func setupTestHelper(tb testing.TB, includeCacheLayer bool, options []app.Option TestLogger: testLogger, } + tb.Cleanup(func() { + if th.IncludeCacheLayer { + // Clean all the caches + appErr := th.App.Srv().InvalidateAllCaches() + require.Nil(tb, appErr) + } + th.Server.Shutdown() + }) + return th } @@ -156,35 +162,30 @@ func (th *TestHelper) NewPluginAPI(manifest *model.Manifest) plugin.API { return th.App.NewPluginAPI(th.Context, manifest) } -func (th *TestHelper) InitBasic() *TestHelper { - th.SystemAdminUser, _ = th.App.CreateUser(th.Context, &model.User{Email: model.NewId() + "success+test@simulator.amazonses.com", Nickname: "Corey Hulen", Password: "passwd1", EmailVerified: true, Roles: model.SystemAdminRoleId}) +func (th *TestHelper) InitBasic(tb testing.TB) *TestHelper { + tb.Helper() - user, _ := th.App.CreateUser(th.Context, &model.User{Email: model.NewId() + "success+test@simulator.amazonses.com", Nickname: "Corey Hulen", Password: "passwd1", EmailVerified: true, Roles: model.SystemUserRoleId}) + var appErr *model.AppError + th.SystemAdminUser, appErr = th.App.CreateUser(th.Context, &model.User{Email: model.NewId() + "success+test@simulator.amazonses.com", Nickname: "Corey Hulen", Password: "passwd1", EmailVerified: true, Roles: model.SystemAdminRoleId}) + require.Nil(tb, appErr) - team, _ := th.App.CreateTeam(th.Context, &model.Team{DisplayName: "Name", Name: "z-z-" + model.NewId() + "a", Email: user.Email, Type: model.TeamOpen}) + th.BasicUser, appErr = th.App.CreateUser(th.Context, &model.User{Email: model.NewId() + "success+test@simulator.amazonses.com", Nickname: "Corey Hulen", Password: "passwd1", EmailVerified: true, Roles: model.SystemUserRoleId}) + require.Nil(tb, appErr) - th.App.JoinUserToTeam(th.Context, team, user, "") + th.BasicTeam, appErr = th.App.CreateTeam(th.Context, &model.Team{DisplayName: "Name", Name: "z-z-" + model.NewId() + "a", Email: th.BasicUser.Email, Type: model.TeamOpen}) + require.Nil(tb, appErr) - channel, _ := th.App.CreateChannel(th.Context, &model.Channel{DisplayName: "Test API Name", Name: "zz" + model.NewId() + "a", Type: model.ChannelTypeOpen, TeamId: team.Id, CreatorId: user.Id}, true) + _, appErr = th.App.JoinUserToTeam(th.Context, th.BasicTeam, th.BasicUser, "") + require.Nil(tb, appErr) - th.BasicUser = user - th.BasicChannel = channel - th.BasicTeam = team + th.BasicChannel, appErr = th.App.CreateChannel(th.Context, &model.Channel{DisplayName: "Test API Name", Name: "zz" + model.NewId() + "a", Type: model.ChannelTypeOpen, TeamId: th.BasicTeam.Id, CreatorId: th.BasicUser.Id}, true) + require.Nil(tb, appErr) return th } -func (th *TestHelper) TearDown() { - if th.IncludeCacheLayer { - // Clean all the caches - th.App.Srv().InvalidateAllCaches() - } - th.Server.Shutdown() -} - func TestStaticFilesRequest(t *testing.T) { th := Setup(t).InitPlugins() - defer th.TearDown() pluginID := "com.mattermost.sample" @@ -223,7 +224,8 @@ func TestStaticFilesRequest(t *testing.T) { // Write the plugin.json manifest pluginManifest := `{"id": "com.mattermost.sample", "server": {"executable": "backend.exe"}, "webapp": {"bundle_path":"main.js"}, "settings_schema": {"settings": []}}` - os.WriteFile(filepath.Join(pluginDir, "plugin.json"), []byte(pluginManifest), 0600) + err = os.WriteFile(filepath.Join(pluginDir, "plugin.json"), []byte(pluginManifest), 0600) + require.NoError(t, err) // Activate the plugin manifest, activated, reterr := th.App.GetPluginsEnvironment().Activate(pluginID) @@ -232,7 +234,8 @@ func TestStaticFilesRequest(t *testing.T) { require.True(t, activated) // Verify access to the bundle with requisite headers - req, _ := http.NewRequest("GET", "/static/plugins/com.mattermost.sample/com.mattermost.sample_724ed0e2ebb2b841_bundle.js", nil) + req, err := http.NewRequest("GET", "/static/plugins/com.mattermost.sample/com.mattermost.sample_724ed0e2ebb2b841_bundle.js", nil) + require.NoError(t, err) res := httptest.NewRecorder() th.Web.MainRouter.ServeHTTP(res, req) assert.Equal(t, http.StatusOK, res.Code) @@ -241,7 +244,8 @@ func TestStaticFilesRequest(t *testing.T) { // Verify cached access to the bundle with an If-Modified-Since timestamp in the future future := time.Now().Add(24 * time.Hour) - req, _ = http.NewRequest("GET", "/static/plugins/com.mattermost.sample/com.mattermost.sample_724ed0e2ebb2b841_bundle.js", nil) + req, err = http.NewRequest("GET", "/static/plugins/com.mattermost.sample/com.mattermost.sample_724ed0e2ebb2b841_bundle.js", nil) + require.NoError(t, err) req.Header.Add("If-Modified-Since", future.Format(time.RFC850)) res = httptest.NewRecorder() th.Web.MainRouter.ServeHTTP(res, req) @@ -251,7 +255,8 @@ func TestStaticFilesRequest(t *testing.T) { // Verify access to the bundle with an If-Modified-Since timestamp in the past past := time.Now().Add(-24 * time.Hour) - req, _ = http.NewRequest("GET", "/static/plugins/com.mattermost.sample/com.mattermost.sample_724ed0e2ebb2b841_bundle.js", nil) + req, err = http.NewRequest("GET", "/static/plugins/com.mattermost.sample/com.mattermost.sample_724ed0e2ebb2b841_bundle.js", nil) + require.NoError(t, err) req.Header.Add("If-Modified-Since", past.Format(time.RFC850)) res = httptest.NewRecorder() th.Web.MainRouter.ServeHTTP(res, req) @@ -260,7 +265,8 @@ func TestStaticFilesRequest(t *testing.T) { assert.Equal(t, []string{"max-age=31556926, public"}, res.Result().Header[http.CanonicalHeaderKey("Cache-Control")]) // Verify handling of 404. - req, _ = http.NewRequest("GET", "/static/plugins/com.mattermost.sample/404.js", nil) + req, err = http.NewRequest("GET", "/static/plugins/com.mattermost.sample/404.js", nil) + require.NoError(t, err) res = httptest.NewRecorder() th.Web.MainRouter.ServeHTTP(res, req) assert.Equal(t, http.StatusNotFound, res.Code) @@ -270,7 +276,6 @@ func TestStaticFilesRequest(t *testing.T) { func TestPublicFilesRequest(t *testing.T) { th := Setup(t).InitPlugins() - defer th.TearDown() pluginDir, err := os.MkdirTemp("", "") require.NoError(t, err) @@ -306,12 +311,14 @@ func TestPublicFilesRequest(t *testing.T) { // Write the plugin.json manifest pluginManifest := `{"id": "com.mattermost.sample", "server": {"executable": "backend.exe"}, "settings_schema": {"settings": []}}` - os.WriteFile(filepath.Join(pluginDir, pluginID, "plugin.json"), []byte(pluginManifest), 0600) + err = os.WriteFile(filepath.Join(pluginDir, pluginID, "plugin.json"), []byte(pluginManifest), 0600) + require.NoError(t, err) // Write the test public file helloHTML := `Hello from the static files public folder for the com.mattermost.sample plugin!` htmlFolderPath := filepath.Join(pluginDir, pluginID, "public") - os.MkdirAll(htmlFolderPath, os.ModePerm) + err = os.MkdirAll(htmlFolderPath, os.ModePerm) + require.NoError(t, err) htmlFilePath := filepath.Join(htmlFolderPath, "hello.html") htmlFileErr := os.WriteFile(htmlFilePath, []byte(helloHTML), 0600) @@ -328,17 +335,20 @@ func TestPublicFilesRequest(t *testing.T) { th.App.Channels().SetPluginsEnvironment(env) - req, _ := http.NewRequest("GET", "/plugins/com.mattermost.sample/public/hello.html", nil) + req, err := http.NewRequest("GET", "/plugins/com.mattermost.sample/public/hello.html", nil) + require.NoError(t, err) res := httptest.NewRecorder() th.Web.MainRouter.ServeHTTP(res, req) assert.Equal(t, helloHTML, res.Body.String()) - req, _ = http.NewRequest("GET", "/plugins/com.mattermost.sample/nefarious-file-access.html", nil) + req, err = http.NewRequest("GET", "/plugins/com.mattermost.sample/nefarious-file-access.html", nil) + require.NoError(t, err) res = httptest.NewRecorder() th.Web.MainRouter.ServeHTTP(res, req) assert.Equal(t, 404, res.Code) - req, _ = http.NewRequest("GET", "/plugins/com.mattermost.sample/public/../nefarious-file-access.html", nil) + req, err = http.NewRequest("GET", "/plugins/com.mattermost.sample/public/../nefarious-file-access.html", nil) + require.NoError(t, err) res = httptest.NewRecorder() th.Web.MainRouter.ServeHTTP(res, req) assert.Equal(t, 301, res.Code) @@ -360,9 +370,9 @@ func TestStatic(t *testing.T) { func TestStaticFilesCaching(t *testing.T) { th := Setup(t).InitPlugins() - defer th.TearDown() - wd, _ := os.Getwd() + wd, err := os.Getwd() + require.NoError(t, err) cmd := exec.Command("ls", path.Join(wd, "client", "plugins")) cmd.Stdout = os.Stdout cmd.Run() @@ -376,7 +386,7 @@ func TestStaticFilesCaching(t *testing.T) { fakeMainBundle := `module.exports = 'main';` fakeRemoteEntry := `module.exports = 'remote';` - err := os.WriteFile("./client/root.html", []byte(fakeRootHTML), 0600) + err = os.WriteFile("./client/root.html", []byte(fakeRootHTML), 0600) require.NoError(t, err) err = os.WriteFile("./client/"+fakeMainBundleName, []byte(fakeMainBundle), 0600) require.NoError(t, err) @@ -388,7 +398,8 @@ func TestStaticFilesCaching(t *testing.T) { err = os.WriteFile("./client/products/boards/remote_entry.js", []byte(fakeRemoteEntry), 0600) require.NoError(t, err) - req, _ := http.NewRequest("GET", "/", nil) + req, err := http.NewRequest("GET", "/", nil) + require.NoError(t, err) res := httptest.NewRecorder() th.Web.MainRouter.ServeHTTP(res, req) require.Equal(t, http.StatusOK, res.Code) @@ -396,28 +407,32 @@ func TestStaticFilesCaching(t *testing.T) { require.Equal(t, []string{"no-cache, max-age=31556926, public"}, res.Result().Header[http.CanonicalHeaderKey("Cache-Control")]) // Checking for HEAD method as well. - req, _ = http.NewRequest(http.MethodHead, "/", nil) + req, err = http.NewRequest(http.MethodHead, "/", nil) + require.NoError(t, err) res = httptest.NewRecorder() th.Web.MainRouter.ServeHTTP(res, req) require.Equal(t, http.StatusOK, res.Code) require.Equal(t, fakeRootHTML, res.Body.String()) require.Equal(t, []string{"no-cache, max-age=31556926, public"}, res.Result().Header[http.CanonicalHeaderKey("Cache-Control")]) - req, _ = http.NewRequest("GET", "/static/"+fakeMainBundleName, nil) + req, err = http.NewRequest("GET", "/static/"+fakeMainBundleName, nil) + require.NoError(t, err) res = httptest.NewRecorder() th.Web.MainRouter.ServeHTTP(res, req) require.Equal(t, http.StatusOK, res.Code) require.Equal(t, fakeMainBundle, res.Body.String()) require.Equal(t, []string{"max-age=31556926, public"}, res.Result().Header[http.CanonicalHeaderKey("Cache-Control")]) - req, _ = http.NewRequest("GET", "/static/remote_entry.js", nil) + req, err = http.NewRequest("GET", "/static/remote_entry.js", nil) + require.NoError(t, err) res = httptest.NewRecorder() th.Web.MainRouter.ServeHTTP(res, req) require.Equal(t, http.StatusOK, res.Code) require.Equal(t, fakeRemoteEntry, res.Body.String()) require.Equal(t, []string{"no-cache, max-age=31556926, public"}, res.Result().Header[http.CanonicalHeaderKey("Cache-Control")]) - req, _ = http.NewRequest("GET", "/static/products/boards/remote_entry.js", nil) + req, err = http.NewRequest("GET", "/static/products/boards/remote_entry.js", nil) + require.NoError(t, err) res = httptest.NewRecorder() th.Web.MainRouter.ServeHTTP(res, req) require.Equal(t, http.StatusOK, res.Code) diff --git a/server/channels/web/webhook_test.go b/server/channels/web/webhook_test.go index 567e8861bb..dafdb43ee0 100644 --- a/server/channels/web/webhook_test.go +++ b/server/channels/web/webhook_test.go @@ -17,8 +17,7 @@ import ( ) func TestIncomingWebhook(t *testing.T) { - th := Setup(t).InitBasic() - defer th.TearDown() + th := Setup(t).InitBasic(t) if !*th.App.Config().ServiceSettings.EnableIncomingWebhooks { _, err := http.Post(apiClient.URL+"/hooks/123", "", strings.NewReader("123")) @@ -238,8 +237,7 @@ func TestIncomingWebhook(t *testing.T) { } func TestCommandWebhooks(t *testing.T) { - th := Setup(t).InitBasic() - defer th.TearDown() + th := Setup(t).InitBasic(t) cmd, appErr := th.App.CreateCommand(&model.Command{ CreatorId: th.BasicUser.Id, diff --git a/server/cmd/mattermost/commands/cmdtestlib.go b/server/cmd/mattermost/commands/cmdtestlib.go index 1d32f79e0f..1349cb6d7a 100644 --- a/server/cmd/mattermost/commands/cmdtestlib.go +++ b/server/cmd/mattermost/commands/cmdtestlib.go @@ -37,13 +37,13 @@ type testHelper struct { } // Setup creates an instance of testHelper. -func Setup(t testing.TB) *testHelper { +func Setup(tb testing.TB) *testHelper { dir, err := testlib.SetupTestResources() if err != nil { panic("failed to create temporary directory: " + err.Error()) } - api4TestHelper := api4.Setup(t) + api4TestHelper := api4.Setup(tb) testHelper := &testHelper{ TestHelper: api4TestHelper, @@ -59,13 +59,13 @@ func Setup(t testing.TB) *testHelper { } // Setup creates an instance of testHelper. -func SetupWithStoreMock(t testing.TB) *testHelper { +func SetupWithStoreMock(tb testing.TB) *testHelper { dir, err := testlib.SetupTestResources() if err != nil { panic("failed to create temporary directory: " + err.Error()) } - api4TestHelper := api4.SetupWithStoreMock(t) + api4TestHelper := api4.SetupWithStoreMock(tb) systemStore := mocks.SystemStore{} systemStore.On("Get").Return(make(model.StringMap), nil) licenseStore := mocks.LicenseStore{} diff --git a/server/cmd/mattermost/commands/server_test.go b/server/cmd/mattermost/commands/server_test.go index f13be02f16..9a6a852e7f 100644 --- a/server/cmd/mattermost/commands/server_test.go +++ b/server/cmd/mattermost/commands/server_test.go @@ -27,9 +27,9 @@ type ServerTestHelper struct { } //nolint:golint,unused -func SetupServerTest(t testing.TB) *ServerTestHelper { +func SetupServerTest(tb testing.TB) *ServerTestHelper { if testing.Short() { - t.SkipNow() + tb.SkipNow() } // Build a channel that will be used by the server to receive system signals... interruptChan := make(chan os.Signal, 1) diff --git a/server/public/shared/request/context.go b/server/public/shared/request/context.go index 2458047374..5cecd90b60 100644 --- a/server/public/shared/request/context.go +++ b/server/public/shared/request/context.go @@ -49,8 +49,8 @@ func EmptyContext(logger mlog.LoggerIFace) *Context { // TestContext creates an empty context with a new logger to use in testing where a test helper is // not required. -func TestContext(t testing.TB) *Context { - logger := mlog.CreateConsoleTestLogger(t) +func TestContext(tb testing.TB) *Context { + logger := mlog.CreateConsoleTestLogger(tb) return EmptyContext(logger) }