Various unit test related improvements (#9865)

* api4: improved error handling

* system_store: more logs

* integrate go-junit-report into test-te/test-ee

* add CI_MINIO_HOST and CI_INBUCKET_HOST instead of CI_HOST

* comment re: minio configuration issue

* fix TestStartServerPortUnavailable to pass even when root can bind to :21

* skip TestFindManifest_FolderPermission while running as root

Makefile

@@ -347,23 +347,29 @@ test-server-race: test-te-race test-ee-race ## Checks for race conditions.
 do-cover-file: ## Creates the test coverage report file.
 	@echo "mode: count" > cover.out
 
-test-te: do-cover-file ## Runs tests in the team edition.
+go-junit-report:
+	go get -u github.com/jstemmer/go-junit-report
+
+test-te: go-junit-report do-cover-file ## Runs tests in the team edition.
 	@echo Testing TE
 	@echo "Packages to test: "$(TE_PACKAGES)
 	find . -name 'cprofile*.out' -exec sh -c 'rm "{}"' \;
-	$(GO) test $(GOFLAGS) -run=$(TESTS) $(TESTFLAGS) -p 1 -v -timeout=2000s -covermode=count -coverpkg=$(ALL_PACKAGES_COMMA) -exec $(ROOT)/scripts/test-xprog.sh $(TE_PACKAGES)
+	$(GO) test $(GOFLAGS) -run=$(TESTS) $(TESTFLAGS) -p 1 -v -timeout=2000s -covermode=count -coverpkg=$(ALL_PACKAGES_COMMA) -exec $(ROOT)/scripts/test-xprog.sh $(TE_PACKAGES) | tee output-test-te
+	cat output-test-te | $(GOPATH)/bin/go-junit-report > report-te.xml && rm output-test-te
 	find . -name 'cprofile*.out' -exec sh -c 'tail -n +2 {} >> cover.out ; rm "{}"' \;
 
-test-ee: do-cover-file ## Runs tests in the enterprise edition.
+test-ee: go-junit-report do-cover-file ## Runs tests in the enterprise edition.
-	@echo Testing EE
-
 ifeq ($(BUILD_ENTERPRISE_READY),true)
+	@echo Testing EE
 	@echo "Packages to test: "$(EE_PACKAGES)
 	find . -name 'cprofile*.out' -exec sh -c 'rm "{}"' \;
-	$(GO) test $(GOFLAGS) -run=$(TESTS) $(TESTFLAGSEE) -p 1 -v -timeout=2000s -covermode=count -coverpkg=$(ALL_PACKAGES_COMMA) -exec $(ROOT)/scripts/test-xprog.sh $(EE_PACKAGES)
+	$(GO) test $(GOFLAGS) -run=$(TESTS) $(TESTFLAGSEE) -p 1 -v -timeout=2000s -covermode=count -coverpkg=$(ALL_PACKAGES_COMMA) -exec $(ROOT)/scripts/test-xprog.sh $(EE_PACKAGES) 2>&1 | tee output-test-ee
+	cat output-test-ee | $(GOPATH)/bin/go-junit-report > report-ee.xml && rm output-test-ee
 	find . -name 'cprofile*.out' -exec sh -c 'tail -n +2 {} >> cover.out ; rm "{}"' \;
 	rm -f config/*.crt
 	rm -f config/*.key
+else
+	@echo Skipping EE Tests
 endif
 
 test-server: test-te test-ee ## Runs tests.

api4/apitestlib.go

@@ -218,6 +218,7 @@ func (me *TestHelper) InitBasic() *TestHelper {
 	me.TeamAdminUser = me.CreateUser()
 	me.App.UpdateUserRoles(me.TeamAdminUser.Id, model.SYSTEM_USER_ROLE_ID, false)
 	me.LoginTeamAdmin()
+
 	me.BasicTeam = me.CreateTeam()
 	me.BasicChannel = me.CreatePublicChannel()
 	me.BasicPrivateChannel = me.CreatePrivateChannel()
@@ -285,7 +286,10 @@ func (me *TestHelper) CreateTeamWithClient(client *model.Client4) *model.Team {
 	}
 
 	utils.DisableDebugLogForTest()
-	rteam, _ := client.CreateTeam(team)
+	rteam, resp := client.CreateTeam(team)
+	if resp.Error != nil {
+		panic(resp.Error)
+	}
 	utils.EnableDebugLogForTest()
 	return rteam
 }
@@ -337,7 +341,10 @@ func (me *TestHelper) CreateChannelWithClientAndTeam(client *model.Client4, chan
 	}
 
 	utils.DisableDebugLogForTest()
-	rchannel, _ := client.CreateChannel(channel)
+	rchannel, resp := client.CreateChannel(channel)
+	if resp.Error != nil {
+		panic(resp.Error)
+	}
 	utils.EnableDebugLogForTest()
 	return rchannel
 }
@@ -447,25 +454,37 @@ func (me *TestHelper) LoginSystemAdmin() {
 
 func (me *TestHelper) LoginBasicWithClient(client *model.Client4) {
 	utils.DisableDebugLogForTest()
-	client.Login(me.BasicUser.Email, me.BasicUser.Password)
+	_, resp := client.Login(me.BasicUser.Email, me.BasicUser.Password)
+	if resp.Error != nil {
+		panic(resp.Error)
+	}
 	utils.EnableDebugLogForTest()
 }
 
 func (me *TestHelper) LoginBasic2WithClient(client *model.Client4) {
 	utils.DisableDebugLogForTest()
-	client.Login(me.BasicUser2.Email, me.BasicUser2.Password)
+	_, resp := client.Login(me.BasicUser2.Email, me.BasicUser2.Password)
+	if resp.Error != nil {
+		panic(resp.Error)
+	}
 	utils.EnableDebugLogForTest()
 }
 
 func (me *TestHelper) LoginTeamAdminWithClient(client *model.Client4) {
 	utils.DisableDebugLogForTest()
-	client.Login(me.TeamAdminUser.Email, me.TeamAdminUser.Password)
+	_, resp := client.Login(me.TeamAdminUser.Email, me.TeamAdminUser.Password)
+	if resp.Error != nil {
+		panic(resp.Error)
+	}
 	utils.EnableDebugLogForTest()
 }
 
 func (me *TestHelper) LoginSystemAdminWithClient(client *model.Client4) {
 	utils.DisableDebugLogForTest()
-	client.Login(me.SystemAdminUser.Email, me.SystemAdminUser.Password)
+	_, resp := client.Login(me.SystemAdminUser.Email, me.SystemAdminUser.Password)
+	if resp.Error != nil {
+		panic(resp.Error)
+	}
 	utils.EnableDebugLogForTest()
 }
 

api4/system_test.go

@@ -430,7 +430,7 @@ func TestEmailTest(t *testing.T) {
 	CheckErrorMessage(t, resp, "api.admin.test_email.missing_server")
 	CheckBadRequestStatus(t, resp)
 
-	inbucket_host := os.Getenv("CI_HOST")
+	inbucket_host := os.Getenv("CI_INBUCKET_HOST")
 	if inbucket_host == "" {
 		inbucket_host = "dockerhost"
 	}
@@ -665,7 +665,7 @@ func TestS3TestConnection(t *testing.T) {
 	defer th.TearDown()
 	Client := th.Client
 
-	s3Host := os.Getenv("CI_HOST")
+	s3Host := os.Getenv("CI_MINIO_HOST")
 	if s3Host == "" {
 		s3Host = "dockerhost"
 	}
@@ -696,6 +696,8 @@ func TestS3TestConnection(t *testing.T) {
 		t.Fatal("should return error - missing s3 bucket")
 	}
 
+	// If this fails, check the test configuration to ensure minio is setup with the
+	// `mattermost-test` bucket defined by model.MINIO_BUCKET.
 	config.FileSettings.AmazonS3Bucket = model.MINIO_BUCKET
 	config.FileSettings.AmazonS3Region = "us-east-1"
 	_, resp = th.SystemAdminClient.TestS3Connection(&config)

app/server_test.go

@@ -5,6 +5,7 @@ package app
 
 import (
 	"crypto/tls"
+	"net"
 	"net/http"
 	"path"
 	"strconv"
@@ -50,9 +51,13 @@ func TestStartServerPortUnavailable(t *testing.T) {
 	a, err := New()
 	require.NoError(t, err)
 
-	// Attempt to listen on a system-reserved port
+	// Listen on the next available port
+	listener, err := net.Listen("tcp", ":0")
+	require.NoError(t, err)
+
+	// Attempt to listen on the port used above.
 	a.UpdateConfig(func(cfg *model.Config) {
-		*cfg.ServiceSettings.ListenAddress = ":21"
+		*cfg.ServiceSettings.ListenAddress = listener.Addr().String()
 	})
 
 	serverErr := a.StartServer()

build/Jenkinsfile

@@ -176,7 +176,7 @@ podTemplate(label: 'jenkins-slave',
     }
     stage('Unit Tests') {
         container('golang') {
-            withEnv(['CI_HOST=localhost', 'CI_INBUCKET_PORT=10080', 'CI_MINIO_PORT=9000']) {
+            withEnv(['CI_INBUCKET_HOST=localhost', 'CI_INBUCKET_PORT=10080', 'CI_MINIO_HOST=localhost', 'CI_MINIO_PORT=9000']) {
                 sh "cd /go/src/github.com/mattermost/mattermost-server && make test-te BUILD_NUMBER=${env.BUILD_NUMBER} TESTFLAGS= TESTFLAGSEE="
             }
         }

model/manifest_test.go

@@ -186,14 +186,19 @@ func TestFindManifest_FileErrors(t *testing.T) {
 }
 
 func TestFindManifest_FolderPermission(t *testing.T) {
+	if os.Geteuid() == 0 {
+		t.Skip("skipping test while running as root: can't effectively remove permissions")
+	}
+
 	for _, tc := range []string{"plugin.yaml", "plugin.json"} {
 		dir, err := ioutil.TempDir("", "mm-plugin-test")
+		require.NoError(t, err)
 		defer os.RemoveAll(dir)
 
 		path := filepath.Join(dir, tc)
 		require.NoError(t, os.Mkdir(path, 0700))
 
-		//User does not have permission in the plugin folder
+		// User does not have permission in the plugin folder
 		err = os.Chmod(dir, 0066)
 		require.NoError(t, err)
 
@@ -202,7 +207,6 @@ func TestFindManifest_FolderPermission(t *testing.T) {
 		assert.Equal(t, "", mpath)
 		assert.Error(t, err, tc)
 		assert.False(t, os.IsNotExist(err), tc)
-
 	}
 }
 

services/filesstore/filesstore_test.go

@@ -56,7 +56,7 @@ func TestS3FileBackendTestSuiteWithEncryption(t *testing.T) {
 }
 
 func runBackendTest(t *testing.T, encrypt bool) {
-	s3Host := os.Getenv("CI_HOST")
+	s3Host := os.Getenv("CI_MINIO_HOST")
 	if s3Host == "" {
 		s3Host = "dockerhost"
 	}

services/mailservice/inbucket.go

@@ -187,7 +187,7 @@ func RetryInbucket(attempts int, callback func() error) (err error) {
 
 func getInbucketHost() (host string) {
 
-	inbucket_host := os.Getenv("CI_HOST")
+	inbucket_host := os.Getenv("CI_INBUCKET_HOST")
 	if inbucket_host == "" {
 		inbucket_host = "dockerhost"
 	}

store/sqlstore/system_store.go

@@ -41,11 +41,11 @@ func (s SqlSystemStore) SaveOrUpdate(system *model.System) store.StoreChannel {
 	return store.Do(func(result *store.StoreResult) {
 		if err := s.GetReplica().SelectOne(&model.System{}, "SELECT * FROM Systems WHERE Name = :Name", map[string]interface{}{"Name": system.Name}); err == nil {
 			if _, err := s.GetMaster().Update(system); err != nil {
-				result.Err = model.NewAppError("SqlSystemStore.SaveOrUpdate", "store.sql_system.update.app_error", nil, "", http.StatusInternalServerError)
+				result.Err = model.NewAppError("SqlSystemStore.SaveOrUpdate", "store.sql_system.update.app_error", nil, err.Error(), http.StatusInternalServerError)
 			}
 		} else {
 			if err := s.GetMaster().Insert(system); err != nil {
-				result.Err = model.NewAppError("SqlSystemStore.SaveOrUpdate", "store.sql_system.save.app_error", nil, "", http.StatusInternalServerError)
+				result.Err = model.NewAppError("SqlSystemStore.SaveOrUpdate", "store.sql_system.save.app_error", nil, err.Error(), http.StatusInternalServerError)
 			}
 		}
 	})
@@ -54,7 +54,7 @@ func (s SqlSystemStore) SaveOrUpdate(system *model.System) store.StoreChannel {
 func (s SqlSystemStore) Update(system *model.System) store.StoreChannel {
 	return store.Do(func(result *store.StoreResult) {
 		if _, err := s.GetMaster().Update(system); err != nil {
-			result.Err = model.NewAppError("SqlSystemStore.Update", "store.sql_system.update.app_error", nil, "", http.StatusInternalServerError)
+			result.Err = model.NewAppError("SqlSystemStore.Update", "store.sql_system.update.app_error", nil, err.Error(), http.StatusInternalServerError)
 		}
 	})
 }
@@ -64,7 +64,7 @@ func (s SqlSystemStore) Get() store.StoreChannel {
 		var systems []model.System
 		props := make(model.StringMap)
 		if _, err := s.GetReplica().Select(&systems, "SELECT * FROM Systems"); err != nil {
-			result.Err = model.NewAppError("SqlSystemStore.Get", "store.sql_system.get.app_error", nil, "", http.StatusInternalServerError)
+			result.Err = model.NewAppError("SqlSystemStore.Get", "store.sql_system.get.app_error", nil, err.Error(), http.StatusInternalServerError)
 		} else {
 			for _, prop := range systems {
 				props[prop.Name] = prop.Value
@@ -79,7 +79,7 @@ func (s SqlSystemStore) GetByName(name string) store.StoreChannel {
 	return store.Do(func(result *store.StoreResult) {
 		var system model.System
 		if err := s.GetReplica().SelectOne(&system, "SELECT * FROM Systems WHERE Name = :Name", map[string]interface{}{"Name": name}); err != nil {
-			result.Err = model.NewAppError("SqlSystemStore.GetByName", "store.sql_system.get_by_name.app_error", nil, "", http.StatusInternalServerError)
+			result.Err = model.NewAppError("SqlSystemStore.GetByName", "store.sql_system.get_by_name.app_error", nil, err.Error(), http.StatusInternalServerError)
 		}
 
 		result.Data = &system
@@ -90,7 +90,7 @@ func (s SqlSystemStore) PermanentDeleteByName(name string) store.StoreChannel {
 	return store.Do(func(result *store.StoreResult) {
 		var system model.System
 		if _, err := s.GetMaster().Exec("DELETE FROM Systems WHERE Name = :Name", map[string]interface{}{"Name": name}); err != nil {
-			result.Err = model.NewAppError("SqlSystemStore.PermanentDeleteByName", "store.sql_system.permanent_delete_by_name.app_error", nil, "", http.StatusInternalServerError)
+			result.Err = model.NewAppError("SqlSystemStore.PermanentDeleteByName", "store.sql_system.permanent_delete_by_name.app_error", nil, err.Error(), http.StatusInternalServerError)
 		}
 
 		result.Data = &system