Enable the errorAssertions govet check for mattermost-server code (#1…

…7346)

* Enable the errorAssertions govet check for mattermost-server code

* Removing unnecesary change

* Fixing some tests

* Fixing tests

* Fixing more after merge

* Fixing new offending entries

* Fixing small vet checks

* Fixing new cases detected by govet

* Fixing remote_cluster_test errors

* Fixing assertion

Co-authored-by: Mattermod <mattermod@users.noreply.github.com>

Makefile

@@ -603,7 +603,7 @@ vet: ## Run mattermost go vet specific checks
 		echo "mattermost-govet is not installed. Please install it executing \"GO111MODULE=off GOBIN=$(PWD)/bin go get -u github.com/mattermost/mattermost-govet\""; \
 		exit 1; \
 	fi;
-	@VET_CMD="-license -structuredLogging -inconsistentReceiverName -inconsistentReceiverName.ignore=session_serial_gen.go,team_member_serial_gen.go,user_serial_gen.go -emptyStrCmp -tFatal -configtelemetry"; \
+	@VET_CMD="-license -structuredLogging -inconsistentReceiverName -inconsistentReceiverName.ignore=session_serial_gen.go,team_member_serial_gen.go,user_serial_gen.go -emptyStrCmp -tFatal -configtelemetry -errorAssertions"; \
 	if ! [ -z "${MM_VET_OPENSPEC_PATH}" ] && [ -f "${MM_VET_OPENSPEC_PATH}" ]; then \
 		VET_CMD="$$VET_CMD -openApiSync -openApiSync.spec=$$MM_VET_OPENSPEC_PATH"; \
 	else \

api4/file_test.go

@@ -1085,37 +1085,37 @@ func TestSearchFiles(t *testing.T) {
 	Client := th.Client
 
 	filename := "search for fileInfo1"
-	fileInfo1, err := th.App.UploadFile(data, th.BasicChannel.Id, filename)
-	require.Nil(t, err)
+	fileInfo1, appErr := th.App.UploadFile(data, th.BasicChannel.Id, filename)
+	require.Nil(t, appErr)
 	err = th.App.Srv().Store.FileInfo().AttachToPost(fileInfo1.Id, th.BasicPost.Id, th.BasicUser.Id)
-	require.Nil(t, err)
+	require.NoError(t, err)
 
 	filename = "search for fileInfo2"
-	fileInfo2, err := th.App.UploadFile(data, th.BasicChannel.Id, filename)
-	require.Nil(t, err)
+	fileInfo2, appErr := th.App.UploadFile(data, th.BasicChannel.Id, filename)
+	require.Nil(t, appErr)
 	err = th.App.Srv().Store.FileInfo().AttachToPost(fileInfo2.Id, th.BasicPost.Id, th.BasicUser.Id)
-	require.Nil(t, err)
+	require.NoError(t, err)
 
 	filename = "tagged search for fileInfo3"
-	fileInfo3, err := th.App.UploadFile(data, th.BasicChannel.Id, filename)
-	require.Nil(t, err)
+	fileInfo3, appErr := th.App.UploadFile(data, th.BasicChannel.Id, filename)
+	require.Nil(t, appErr)
 	err = th.App.Srv().Store.FileInfo().AttachToPost(fileInfo3.Id, th.BasicPost.Id, th.BasicUser.Id)
-	require.Nil(t, err)
+	require.NoError(t, err)
 
 	filename = "tagged for fileInfo4"
-	fileInfo4, err := th.App.UploadFile(data, th.BasicChannel.Id, filename)
-	require.Nil(t, err)
+	fileInfo4, appErr := th.App.UploadFile(data, th.BasicChannel.Id, filename)
+	require.Nil(t, appErr)
 	err = th.App.Srv().Store.FileInfo().AttachToPost(fileInfo4.Id, th.BasicPost.Id, th.BasicUser.Id)
-	require.Nil(t, err)
+	require.NoError(t, err)
 
 	archivedChannel := th.CreatePublicChannel()
-	fileInfo5, err := th.App.UploadFile(data, archivedChannel.Id, "tagged for fileInfo3")
-	require.Nil(t, err)
+	fileInfo5, appErr := th.App.UploadFile(data, archivedChannel.Id, "tagged for fileInfo3")
+	require.Nil(t, appErr)
 	post := &model.Post{ChannelId: archivedChannel.Id, Message: model.NewId() + "a"}
 	rpost, resp := Client.CreatePost(post)
 	CheckNoError(t, resp)
 	err = th.App.Srv().Store.FileInfo().AttachToPost(fileInfo5.Id, rpost.Id, th.BasicUser.Id)
-	require.Nil(t, err)
+	require.NoError(t, err)
 	th.Client.DeleteChannel(archivedChannel.Id)
 
 	terms := "search"

api4/team_test.go

@@ -2926,7 +2926,7 @@ func TestInviteGuestsToTeam(t *testing.T) {
 
 	t.Run("invalid data in request body", func(t *testing.T) {
 		res, err := th.SystemAdminClient.DoApiPost(th.SystemAdminClient.GetTeamRoute(th.BasicTeam.Id)+"/invite-guests/email", "bad data")
-		require.Error(t, err)
+		require.NotNil(t, err)
 		require.Equal(t, "api.team.invite_guests_to_channels.invalid_body.app_error", err.Id)
 		require.Equal(t, http.StatusBadRequest, res.StatusCode)
 	})

api4/user_test.go

@@ -3384,7 +3384,7 @@ func TestLoginWithLag(t *testing.T) {
 		defer mainHelper.ToggleReplicasOff()
 
 		cmdErr := mainHelper.SetReplicationLagForTesting(5)
-		require.Nil(t, cmdErr)
+		require.NoError(t, cmdErr)
 		defer mainHelper.SetReplicationLagForTesting(0)
 
 		_, resp := th.Client.Login(th.BasicUser.Email, th.BasicUser.Password)
@@ -6083,7 +6083,7 @@ func TestSetProfileImageWithProviderAttributes(t *testing.T) {
 	doCleanup := func(t *testing.T, th *TestHelper, user *model.User) {
 		info := &model.FileInfo{Path: "users/" + user.Id + "/profile.png"}
 		err = th.cleanupTestFile(info)
-		require.Nil(t, err)
+		require.NoError(t, err)
 	}
 
 	t.Run("LDAP user", func(t *testing.T) {

app/channel_test.go

@@ -1689,18 +1689,19 @@ func TestPatchChannelModerationsForChannel(t *testing.T) {
 			},
 		},
 		{
-			Name: "Removing manage members from guests role should error",
+			Name: "Removing manage members from guests role should not error",
 			ChannelModerationsPatch: []*model.ChannelModerationPatch{
 				{
 					Name:  &manageMembers,
 					Roles: &model.ChannelModeratedRolesPatch{Guests: model.NewBool(false)},
 				},
 			},
 			PermissionsModeratedByPatch: map[string]*model.ChannelModeratedRoles{},
-			ShouldError:                 true,
+			ShouldError:                 false,
+			ShouldHaveNoChannelScheme:   true,
 		},
 		{
-			Name: "Removing a permission that is not channel moderated should error",
+			Name: "Removing a permission that is not channel moderated should not error",
 			ChannelModerationsPatch: []*model.ChannelModerationPatch{
 				{
 					Name: &nonChannelModeratedPermission,
@@ -1711,7 +1712,8 @@ func TestPatchChannelModerationsForChannel(t *testing.T) {
 				},
 			},
 			PermissionsModeratedByPatch: map[string]*model.ChannelModeratedRoles{},
-			ShouldError:                 true,
+			ShouldError:                 false,
+			ShouldHaveNoChannelScheme:   true,
 		},
 		{
 			Name: "Error when adding a permission that is disabled in the parent member role",
@@ -1829,12 +1831,12 @@ func TestPatchChannelModerationsForChannel(t *testing.T) {
 				}
 			}
 
-			moderations, err := th.App.PatchChannelModerationsForChannel(channel, tc.ChannelModerationsPatch)
+			moderations, appErr := th.App.PatchChannelModerationsForChannel(channel, tc.ChannelModerationsPatch)
 			if tc.ShouldError {
-				require.Error(t, err)
+				require.NotNil(t, appErr)
 				return
 			}
-			require.Nil(t, err)
+			require.Nil(t, appErr)
 
 			updatedChannel, _ := th.App.GetChannel(channel.Id)
 			if tc.ShouldHaveNoChannelScheme {

app/file_test.go

@@ -283,7 +283,7 @@ func TestCreateZipFileAndAddFiles(t *testing.T) {
 
 	err := th.App.CreateZipFileAndAddFiles(&mockBackend, []model.FileData{}, "zip-file-name-to-heaven.zip", "directory-to-heaven")
 
-	require.NotNil(t, err)
+	require.Error(t, err)
 	require.Equal(t, err.Error(), "only those who dare to fail greatly can ever achieve greatly")
 
 	mockBackend = filesStoreMocks.FileBackend{}
@@ -371,7 +371,7 @@ func TestSearchFilesInTeamForUser(t *testing.T) {
 			})
 			time.Sleep(1 * time.Millisecond)
 
-			require.Nil(t, err)
+			require.NoError(t, err)
 
 			fileInfos[i] = fileInfo
 		}

app/post_test.go

@@ -2030,26 +2030,26 @@ func TestReplyToPostWithLag(t *testing.T) {
 
 	t.Run("replication lag time great than reply time", func(t *testing.T) {
 		err := mainHelper.SetReplicationLagForTesting(5)
-		require.Nil(t, err)
+		require.NoError(t, err)
 		defer mainHelper.SetReplicationLagForTesting(0)
 		mainHelper.ToggleReplicasOn()
 		defer mainHelper.ToggleReplicasOff()
 
-		root, err := th.App.CreatePost(&model.Post{
+		root, appErr := th.App.CreatePost(&model.Post{
 			UserId:    th.BasicUser.Id,
 			ChannelId: th.BasicChannel.Id,
 			Message:   "root post",
 		}, th.BasicChannel, false, true)
-		require.Nil(t, err)
+		require.Nil(t, appErr)
 
-		reply, err := th.App.CreatePost(&model.Post{
+		reply, appErr := th.App.CreatePost(&model.Post{
 			UserId:    th.BasicUser2.Id,
 			ChannelId: th.BasicChannel.Id,
 			RootId:    root.Id,
 			ParentId:  root.Id,
 			Message:   fmt.Sprintf("@%s", th.BasicUser2.Username),
 		}, th.BasicChannel, false, true)
-		require.Nil(t, err)
+		require.Nil(t, appErr)
 		require.NotNil(t, reply)
 	})
 }

app/remote_cluster_test.go

@@ -32,7 +32,7 @@ func TestAddRemoteCluster(t *testing.T) {
 
 		remoteCluster.RemoteId = model.NewId()
 		_, err = th.App.AddRemoteCluster(remoteCluster)
-		require.Error(t, err, "Adding a duplicate remote cluster should error")
+		require.NotNil(t, err, "Adding a duplicate remote cluster should error")
 		assert.Contains(t, err.Error(), "Remote cluster has already been added.")
 	})
 
@@ -103,7 +103,7 @@ func TestUpdateRemoteCluster(t *testing.T) {
 		savedRemoteClustered.SiteURL = remoteCluster.SiteURL
 		savedRemoteClustered.RemoteTeamId = remoteCluster.RemoteTeamId
 		_, err = th.App.UpdateRemoteCluster(savedRemoteClustered)
-		require.Error(t, err, "Updating remote cluster with duplicate site url should error")
+		require.NotNil(t, err, "Updating remote cluster with duplicate site url should error")
 		assert.Contains(t, err.Error(), "Remote cluster with the same url already exists.")
 	})
 

app/server_test.go

@@ -173,10 +173,10 @@ func TestStartServerNoS3Bucket(t *testing.T) {
 	require.NoError(t, err)
 
 	// ensure that a new bucket was created
-	backend, err := s.FileBackend()
-	require.Nil(t, err)
+	backend, appErr := s.FileBackend()
+	require.Nil(t, appErr)
 	err = backend.(*filestore.S3FileBackend).TestConnection()
-	require.Nil(t, err)
+	require.NoError(t, err)
 }
 
 func TestStartServerTLSSuccess(t *testing.T) {

app/session_test.go

@@ -465,13 +465,13 @@ func TestGetRemoteClusterSession(t *testing.T) {
 
 	t.Run("Invalid remote token should return error", func(t *testing.T) {
 		session, err := th.App.GetRemoteClusterSession(model.NewId(), remoteId)
-		require.Error(t, err)
+		require.NotNil(t, err)
 		require.Nil(t, session)
 	})
 
 	t.Run("Invalid remote id should return error", func(t *testing.T) {
 		session, err := th.App.GetRemoteClusterSession(token, model.NewId())
-		require.Error(t, err)
+		require.NotNil(t, err)
 		require.Nil(t, session)
 	})
 }

app/user_test.go

@@ -118,7 +118,8 @@ func TestSetDefaultProfileImage(t *testing.T) {
 		Id:       model.NewId(),
 		Username: "notvaliduser",
 	})
-	require.Error(t, err)
+	// It doesn't fail, but it does nothing
+	require.Nil(t, err)
 
 	user := th.BasicUser
 

config/database_test.go

@@ -82,7 +82,7 @@ func getActualDatabaseConfig(t *testing.T) (string, *model.Config) {
 
 		var actualCfg *model.Config
 		err = json.Unmarshal(actual.Value, &actualCfg)
-		require.Nil(t, err)
+		require.NoError(t, err)
 		return actual.ID, actualCfg
 	}
 	var actual struct {
@@ -95,7 +95,7 @@ func getActualDatabaseConfig(t *testing.T) (string, *model.Config) {
 
 	var actualCfg *model.Config
 	err = json.Unmarshal(actual.Value, &actualCfg)
-	require.Nil(t, err)
+	require.NoError(t, err)
 	return actual.ID, actualCfg
 }
 

config/file_test.go

@@ -54,12 +54,12 @@ func getActualFileConfig(t *testing.T, path string) *model.Config {
 	t.Helper()
 
 	f, err := os.Open(path)
-	require.Nil(t, err)
+	require.NoError(t, err)
 	defer f.Close()
 
 	var actualCfg *model.Config
 	err = json.NewDecoder(f).Decode(&actualCfg)
-	require.Nil(t, err)
+	require.NoError(t, err)
 
 	return actualCfg
 }

migrations/migrations_test.go

@@ -35,15 +35,15 @@ func TestGetMigrationState(t *testing.T) {
 		Value: "true",
 	}
 	nErr := th.App.Srv().Store.System().Save(&system)
-	assert.Nil(t, nErr)
+	assert.NoError(t, nErr)
 
 	state, job, err = GetMigrationState(migrationKey, th.App.Srv().Store)
 	assert.Nil(t, err)
 	assert.Nil(t, job)
 	assert.Equal(t, "completed", state)
 
 	_, nErr = th.App.Srv().Store.System().PermanentDeleteByName(migrationKey)
-	assert.Nil(t, nErr)
+	assert.NoError(t, nErr)
 
 	// Test with a job scheduled in "pending" state.
 	j1 := &model.Job{
@@ -57,7 +57,7 @@ func TestGetMigrationState(t *testing.T) {
 	}
 
 	j1, nErr = th.App.Srv().Store.Job().Save(j1)
-	require.Nil(t, nErr)
+	require.NoError(t, nErr)
 
 	state, job, err = GetMigrationState(migrationKey, th.App.Srv().Store)
 	assert.Nil(t, err)
@@ -76,7 +76,7 @@ func TestGetMigrationState(t *testing.T) {
 	}
 
 	j2, nErr = th.App.Srv().Store.Job().Save(j2)
-	require.Nil(t, nErr)
+	require.NoError(t, nErr)
 
 	state, job, err = GetMigrationState(migrationKey, th.App.Srv().Store)
 	assert.Nil(t, err)
@@ -95,7 +95,7 @@ func TestGetMigrationState(t *testing.T) {
 	}
 
 	j3, nErr = th.App.Srv().Store.Job().Save(j3)
-	require.Nil(t, nErr)
+	require.NoError(t, nErr)
 
 	state, job, err = GetMigrationState(migrationKey, th.App.Srv().Store)
 	assert.Nil(t, err)

model/remote_cluster_test.go

@@ -20,8 +20,8 @@ func TestRemoteClusterJson(t *testing.T) {
 	json, err := o.ToJSON()
 	require.NoError(t, err)
 
-	ro, err := RemoteClusterFromJSON(strings.NewReader(json))
-	require.Nil(t, err)
+	ro, appErr := RemoteClusterFromJSON(strings.NewReader(json))
+	require.Nil(t, appErr)
 
 	require.Equal(t, o.RemoteId, ro.RemoteId)
 	require.Equal(t, o.DisplayName, ro.DisplayName)
@@ -72,8 +72,8 @@ func TestRemoteClusterMsgJson(t *testing.T) {
 	json, err := json.Marshal(o)
 	require.NoError(t, err)
 
-	ro, err := RemoteClusterMsgFromJSON(strings.NewReader(string(json)))
-	require.Nil(t, err)
+	ro, appErr := RemoteClusterMsgFromJSON(strings.NewReader(string(json)))
+	require.Nil(t, appErr)
 
 	require.Equal(t, o.Id, ro.Id)
 	require.Equal(t, o.CreateAt, ro.CreateAt)

plugin/helpers_bots_test.go

@@ -572,7 +572,7 @@ func TestShouldProcessMessage(t *testing.T) {
 		api.On("KVGet", plugin.BotUserKey).Return(nil, nil)
 
 		shouldProcessMessage, err := p.ShouldProcessMessage(&model.Post{ChannelId: channelID, UserId: userID}, plugin.BotID(expectedBotID))
-		assert.Nil(t, err)
+		assert.NoError(t, err)
 
 		assert.True(t, shouldProcessMessage)
 	})

services/remotecluster/ping_test.go

@@ -119,7 +119,7 @@ func TestPing(t *testing.T) {
 
 		wg.Wait()
 
-		assert.Nil(t, merr.ErrorOrNil())
+		assert.NoError(t, merr.ErrorOrNil())
 
 		assert.Equal(t, int32(NumRemotes), atomic.LoadInt32(&countWebReq))
 		t.Log(fmt.Sprintf("%d web requests counted;  %d expected",

store/localcachelayer/user_layer_test.go

@@ -279,13 +279,13 @@ func TestUserStoreGetManyCache(t *testing.T) {
 		require.NoError(t, err)
 
 		gotUsers, err := cachedStore.User().GetMany(context.Background(), []string{fakeUser.Id, otherFakeUser.Id})
-		require.Nil(t, err)
+		require.NoError(t, err)
 		assert.Len(t, gotUsers, 2)
 		assert.Contains(t, gotUsers, fakeUser)
 		assert.Contains(t, gotUsers, otherFakeUser)
 
 		gotUsers, err = cachedStore.User().GetMany(context.Background(), []string{fakeUser.Id, otherFakeUser.Id})
-		require.Nil(t, err)
+		require.NoError(t, err)
 		assert.Len(t, gotUsers, 2)
 		mockStore.User().(*mocks.UserStore).AssertNumberOfCalls(t, "GetMany", 1)
 	})
@@ -297,7 +297,7 @@ func TestUserStoreGetManyCache(t *testing.T) {
 		require.NoError(t, err)
 
 		gotUsers, err := cachedStore.User().GetMany(context.Background(), []string{fakeUser.Id, otherFakeUser.Id})
-		require.Nil(t, err)
+		require.NoError(t, err)
 		assert.Len(t, gotUsers, 2)
 		assert.Contains(t, gotUsers, fakeUser)
 		assert.Contains(t, gotUsers, otherFakeUser)