MM-16997: Simplified TestUploadFiles (#11707)

* MM-16997: Simplified TestUploadFiles

Tickets:
https://mattermost.atlassian.net/browse/MM-16997
https://mattermost.atlassian.net/browse/MM-16760

- The tests now fully buffer the data vefore uploading it. The prior
  code was much more complex because it was intended to eventually
  mature into generic client code; not a priority, definitely not
  valuable for this test.
- Also improved error handling in TestHookFileWillBeUploaded for
  MM-16760

* PR feedback: typo

api4/file_test.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
+	"math/rand"
 	"mime/multipart"
 	"net/http"
 	"net/textproto"
@@ -22,6 +23,7 @@ import (
 	"github.com/mattermost/mattermost-server/model"
 	"github.com/mattermost/mattermost-server/utils/fileutils"
 	"github.com/mattermost/mattermost-server/utils/testutils"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
@@ -31,36 +33,6 @@ func init() {
 	testDir, _ = fileutils.FindDir("tests")
 }
 
-func checkCond(tb testing.TB, cond bool, text string) {
-	if !cond {
-		tb.Error(text)
-	}
-}
-
-func checkEq(tb testing.TB, v1, v2 interface{}, text string) {
-	checkCond(tb, fmt.Sprintf("%+v", v1) == fmt.Sprintf("%+v", v2), text)
-}
-
-func checkNeq(tb testing.TB, v1, v2 interface{}, text string) {
-	checkCond(tb, fmt.Sprintf("%+v", v1) != fmt.Sprintf("%+v", v2), text)
-}
-
-type zeroReader struct {
-	limit, read int
-}
-
-func (z *zeroReader) Read(b []byte) (int, error) {
-	for i := range b {
-		if z.read == z.limit {
-			return i, io.EOF
-		}
-		b[i] = 0
-		z.read++
-	}
-
-	return len(b), nil
-}
-
 // File Section
 var quoteEscaper = strings.NewReplacer("\\", "\\\\", `"`, "\\\"")
 
@@ -68,70 +40,112 @@ func escapeQuotes(s string) string {
 	return quoteEscaper.Replace(s)
 }
 
-type UploadOpener func() (io.ReadCloser, int64, error)
-
-func NewUploadOpenerReader(in io.Reader) UploadOpener {
-	return func() (io.ReadCloser, int64, error) {
-		rc, ok := in.(io.ReadCloser)
-		if ok {
-			return rc, -1, nil
-		} else {
-			return ioutil.NopCloser(in), -1, nil
-		}
+func randomBytes(n int) []byte {
+	bb := make([]byte, n)
+	_, err := rand.Read(bb)
+	if err != nil {
+		panic(err.Error())
 	}
+	return bb
 }
 
-func NewUploadOpenerFile(path string) UploadOpener {
-	return func() (io.ReadCloser, int64, error) {
-		fi, err := os.Stat(path)
-		if err != nil {
-			return nil, 0, err
-		}
-		f, err := os.Open(path)
-		if err != nil {
-			return nil, 0, err
-		}
-		return f, fi.Size(), nil
+func fileBytes(path string) []byte {
+	path = filepath.Join(testDir, path)
+	f, err := os.Open(path)
+	if err != nil {
+		panic(err.Error())
 	}
+	defer f.Close()
+	bb, err := ioutil.ReadAll(f)
+	if err != nil {
+		panic(err.Error())
+	}
+	return bb
 }
 
-// testUploadFile and testUploadFiles have been "staged" here, eventually they
-// should move back to being model.Client4 methods, once the specifics of the
-// public API are sorted out.
-func testUploadFile(c *model.Client4, url string, body io.Reader, contentType string,
+func testDoUploadFileRequest(t testing.TB, c *model.Client4, url string, blob []byte, contentType string,
 	contentLength int64) (*model.FileUploadResponse, *model.Response) {
-	rq, _ := http.NewRequest("POST", c.ApiUrl+c.GetFilesRoute()+url, body)
+	req, err := http.NewRequest("POST", c.ApiUrl+c.GetFilesRoute()+url, bytes.NewReader(blob))
+	require.Nil(t, err)
+
 	if contentLength != 0 {
-		rq.ContentLength = contentLength
+		req.ContentLength = contentLength
 	}
-	rq.Header.Set("Content-Type", contentType)
-
+	req.Header.Set("Content-Type", contentType)
 	if len(c.AuthToken) > 0 {
-		rq.Header.Set(model.HEADER_AUTH, c.AuthType+" "+c.AuthToken)
+		req.Header.Set(model.HEADER_AUTH, c.AuthType+" "+c.AuthToken)
 	}
 
-	rp, err := c.HttpClient.Do(rq)
-	if err != nil || rp == nil {
-		return nil, model.BuildErrorResponse(rp, model.NewAppError(url, "model.client.connecting.app_error", nil, err.Error(), 0))
-	}
-	defer closeBody(rp)
+	resp, err := c.HttpClient.Do(req)
+	require.Nil(t, err)
+	require.NotNil(t, resp)
+	defer closeBody(resp)
 
-	if rp.StatusCode >= 300 {
-		return nil, model.BuildErrorResponse(rp, model.AppErrorFromJson(rp.Body))
+	if resp.StatusCode >= 300 {
+		return nil, model.BuildErrorResponse(resp, model.AppErrorFromJson(resp.Body))
 	}
 
-	return model.FileUploadResponseFromJson(rp.Body), model.BuildResponse(rp)
+	return model.FileUploadResponseFromJson(resp.Body), model.BuildResponse(resp)
 }
 
-func testUploadFiles(
+func testUploadFilesPost(
+	t testing.TB,
 	c *model.Client4,
 	channelId string,
 	names []string,
-	openers []UploadOpener,
-	contentLengths []int64,
+	blobs [][]byte,
+	clientIds []string,
+	useChunked bool,
+) (*model.FileUploadResponse, *model.Response) {
+
+	// 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))
+
+	fileUploadResponse := &model.FileUploadResponse{}
+	for i, blob := range blobs {
+		var cl int64
+		if useChunked {
+			cl = -1
+		} else {
+			cl = int64(len(blob))
+		}
+		ct := http.DetectContentType(blob)
+
+		postURL := fmt.Sprintf("?channel_id=%v", url.QueryEscape(channelId)) +
+			fmt.Sprintf("&filename=%v", url.QueryEscape(names[i]))
+		if len(clientIds) > i {
+			postURL += fmt.Sprintf("&client_id=%v", url.QueryEscape(clientIds[i]))
+		}
+
+		fur, resp := testDoUploadFileRequest(t, c, postURL, blob, ct, cl)
+		if resp.Error != nil {
+			return nil, resp
+		}
+
+		fileUploadResponse.FileInfos = append(fileUploadResponse.FileInfos, fur.FileInfos[0])
+		if len(clientIds) > 0 {
+			if len(fur.ClientIds) > 0 {
+				fileUploadResponse.ClientIds = append(fileUploadResponse.ClientIds, fur.ClientIds[0])
+			} else {
+				fileUploadResponse.ClientIds = append(fileUploadResponse.ClientIds, "")
+			}
+		}
+	}
+
+	return fileUploadResponse, nil
+}
+
+func testUploadFilesMultipart(
+	t testing.TB,
+	c *model.Client4,
+	channelId string,
+	names []string,
+	blobs [][]byte,
 	clientIds []string,
-	useMultipart,
-	useChunkedInSimplePost bool,
 ) (
 	fileUploadResponse *model.FileUploadResponse,
 	response *model.Response,
@@ -139,167 +153,38 @@ func testUploadFiles(
 	// 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.
-	if len(names) == 0 || len(openers) == 0 || len(names) != len(openers) {
-		return nil, &model.Response{
-			Error: model.NewAppError("testUploadFiles",
-				"model.client.upload_post_attachment.file.app_error",
-				nil, "Empty or mismatched file data", http.StatusBadRequest),
+	require.NotEmpty(t, names)
+	require.NotEmpty(t, blobs)
+	require.Equal(t, len(names), len(blobs))
+
+	mwBody := &bytes.Buffer{}
+	mw := multipart.NewWriter(mwBody)
+
+	err := mw.WriteField("channel_id", channelId)
+	require.Nil(t, err)
+
+	for i, blob := range blobs {
+		ct := http.DetectContentType(blob)
+		if len(clientIds) > i {
+			err = mw.WriteField("client_ids", clientIds[i])
+			require.Nil(t, err)
 		}
+
+		h := textproto.MIMEHeader{}
+		h.Set("Content-Disposition",
+			fmt.Sprintf(`form-data; name="files"; filename="%s"`, escapeQuotes(names[i])))
+		h.Set("Content-Type", ct)
+
+		// If we error here, writing to mw, the deferred handler
+		part, err := mw.CreatePart(h)
+		require.Nil(t, err)
+
+		_, err = io.Copy(part, bytes.NewReader(blob))
+		require.Nil(t, err)
 	}
 
-	// emergencyResponse is a convenience wrapper to return an error response
-	emergencyResponse := func(err error, errCode string) *model.Response {
-		return &model.Response{
-			Error: model.NewAppError("testUploadFiles",
-				"model.client."+errCode+".app_error",
-				nil, err.Error(), http.StatusBadRequest),
-		}
-	}
-
-	// For multipart, start writing the request as a goroutine, and pipe
-	// multipart.Writer into it, otherwise generate a new request each
-	// time.
-	pipeReader, pipeWriter := io.Pipe()
-	mw := multipart.NewWriter(pipeWriter)
-
-	if useMultipart {
-		fileUploadResponseChannel := make(chan *model.FileUploadResponse)
-		responseChannel := make(chan *model.Response)
-		closedMultipart := false
-
-		go func() {
-			fur, resp := testUploadFile(c, "", pipeReader, mw.FormDataContentType(), -1)
-			responseChannel <- resp
-			fileUploadResponseChannel <- fur
-		}()
-
-		defer func() {
-			for {
-				select {
-				// Premature response, before the entire
-				// multipart was sent
-				case response = <-responseChannel:
-					// Guaranteed to be there
-					fileUploadResponse = <-fileUploadResponseChannel
-					if !closedMultipart {
-						_ = mw.Close()
-						_ = pipeWriter.Close()
-						closedMultipart = true
-					}
-					return
-
-				// Normal response, after the multipart was sent.
-				default:
-					if !closedMultipart {
-						err := mw.Close()
-						if err != nil {
-							fileUploadResponse = nil
-							response = emergencyResponse(err, "upload_post_attachment.writer")
-							return
-						}
-						err = pipeWriter.Close()
-						if err != nil {
-							fileUploadResponse = nil
-							response = emergencyResponse(err, "upload_post_attachment.writer")
-							return
-						}
-						closedMultipart = true
-					}
-				}
-			}
-		}()
-
-		err := mw.WriteField("channel_id", channelId)
-		if err != nil {
-			return nil, emergencyResponse(err, "upload_post_attachment.channel_id")
-		}
-	} else {
-		fileUploadResponse = &model.FileUploadResponse{}
-	}
-
-	data := make([]byte, 512)
-
-	upload := func(i int, f io.ReadCloser) *model.Response {
-		var cl int64
-		defer f.Close()
-
-		if len(contentLengths) > i {
-			cl = contentLengths[i]
-		}
-
-		n, err := f.Read(data)
-		if err != nil && err != io.EOF {
-			return emergencyResponse(err, "upload_post_attachment")
-		}
-		ct := http.DetectContentType(data[:n])
-		reader := io.MultiReader(bytes.NewReader(data[:n]), f)
-
-		if useMultipart {
-			if len(clientIds) > i {
-				err := mw.WriteField("client_ids", clientIds[i])
-				if err != nil {
-					return emergencyResponse(err, "upload_post_attachment.file")
-				}
-			}
-
-			h := make(textproto.MIMEHeader)
-			h.Set("Content-Disposition",
-				fmt.Sprintf(`form-data; name="files"; filename="%s"`, escapeQuotes(names[i])))
-			h.Set("Content-Type", ct)
-
-			// If we error here, writing to mw, the deferred handler
-			part, err := mw.CreatePart(h)
-			if err != nil {
-				return emergencyResponse(err, "upload_post_attachment.writer")
-			}
-
-			_, err = io.Copy(part, reader)
-			if err != nil {
-				return emergencyResponse(err, "upload_post_attachment.writer")
-			}
-		} else {
-			postURL := fmt.Sprintf("?channel_id=%v", url.QueryEscape(channelId)) +
-				fmt.Sprintf("&filename=%v", url.QueryEscape(names[i]))
-			if len(clientIds) > i {
-				postURL += fmt.Sprintf("&client_id=%v", url.QueryEscape(clientIds[i]))
-			}
-			if useChunkedInSimplePost {
-				cl = -1
-			}
-			fur, resp := testUploadFile(c, postURL, reader, ct, cl)
-			if resp.Error != nil {
-				return resp
-			}
-			fileUploadResponse.FileInfos = append(fileUploadResponse.FileInfos, fur.FileInfos[0])
-			if len(clientIds) > 0 {
-				if len(fur.ClientIds) > 0 {
-					fileUploadResponse.ClientIds = append(fileUploadResponse.ClientIds, fur.ClientIds[0])
-				} else {
-					fileUploadResponse.ClientIds = append(fileUploadResponse.ClientIds, "")
-				}
-			}
-			response = resp
-		}
-
-		return nil
-	}
-
-	for i, open := range openers {
-		f, _, err := open()
-		if err != nil {
-			return nil, emergencyResponse(err, "upload_post_attachment")
-		}
-
-		resp := upload(i, f)
-		if resp != nil && resp.Error != nil {
-			return nil, resp
-		}
-	}
-
-	// In case of a simple POST, the return values have been set by upload(),
-	// otherwise we finished writing the multipart, and the return values will
-	// be set in defer
-	return fileUploadResponse, response
+	mw.Close()
+	return testDoUploadFileRequest(t, c, "", mwBody.Bytes(), mw.FormDataContentType(), -1)
 }
 
 func TestUploadFiles(t *testing.T) {
@@ -315,14 +200,10 @@ func TestUploadFiles(t *testing.T) {
 	// Get better error messages
 	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableDeveloper = true })
 
-	op := func(name string) UploadOpener {
-		return NewUploadOpenerFile(filepath.Join(testDir, name))
-	}
-
 	tests := []struct {
 		title     string
 		client    *model.Client4
-		openers   []UploadOpener
+		blobs     [][]byte
 		names     []string
 		clientIds []string
 
@@ -370,7 +251,7 @@ func TestUploadFiles(t *testing.T) {
 		{
 			title:                 "Happy invalid image",
 			names:                 []string{"testgif.gif"},
-			openers:               []UploadOpener{NewUploadOpenerFile(filepath.Join(testDir, "test-search.md"))},
+			blobs:                 [][]byte{fileBytes("test-search.md")},
 			skipPayloadValidation: true,
 			expectedCreatorId:     th.BasicUser.Id,
 		},
@@ -500,11 +381,11 @@ func TestUploadFiles(t *testing.T) {
 			title:                  "Happy stream",
 			useChunkedInSimplePost: true,
 			skipPayloadValidation:  true,
-			names:                  []string{"50Mb-stream"},
-			openers:                []UploadOpener{NewUploadOpenerReader(&zeroReader{limit: 50 * 1024 * 1024})},
+			names:                  []string{"1Mb-stream"},
+			blobs:                  [][]byte{randomBytes(1024 * 1024)},
 			setupConfig: func(a *app.App) func(a *app.App) {
 				maxFileSize := *a.Config().FileSettings.MaxFileSize
-				a.UpdateConfig(func(cfg *model.Config) { *cfg.FileSettings.MaxFileSize = 50 * 1024 * 1024 })
+				a.UpdateConfig(func(cfg *model.Config) { *cfg.FileSettings.MaxFileSize = 1024 * 1024 })
 				return func(a *app.App) {
 					a.UpdateConfig(func(cfg *model.Config) { *cfg.FileSettings.MaxFileSize = maxFileSize })
 				}
@@ -598,13 +479,13 @@ func TestUploadFiles(t *testing.T) {
 		{
 			title:                 "Error stream too large",
 			skipPayloadValidation: true,
-			names:                 []string{"50Mb-stream"},
-			openers:               []UploadOpener{NewUploadOpenerReader(&zeroReader{limit: 50 * 1024 * 1024})},
+			names:                 []string{"1Mb-stream"},
+			blobs:                 [][]byte{randomBytes(1024 * 1024)},
 			skipSuccessValidation: true,
 			checkResponse:         CheckRequestEntityTooLargeStatus,
 			setupConfig: func(a *app.App) func(a *app.App) {
 				maxFileSize := *a.Config().FileSettings.MaxFileSize
-				a.UpdateConfig(func(cfg *model.Config) { *cfg.FileSettings.MaxFileSize = 100 * 1024 })
+				a.UpdateConfig(func(cfg *model.Config) { *cfg.FileSettings.MaxFileSize = 10 * 1024 })
 				return func(a *app.App) {
 					a.UpdateConfig(func(cfg *model.Config) { *cfg.FileSettings.MaxFileSize = maxFileSize })
 				}
@@ -618,19 +499,6 @@ func TestUploadFiles(t *testing.T) {
 				continue
 			}
 
-			// Set the default values and title
-			client := th.Client
-			if tc.client != nil {
-				client = tc.client
-			}
-			channelId := channel.Id
-			if tc.channelId != "" {
-				channelId = tc.channelId
-			}
-			if tc.checkResponse == nil {
-				tc.checkResponse = CheckNoError
-			}
-
 			title := ""
 			if useMultipart {
 				title = "multipart "
@@ -642,22 +510,47 @@ func TestUploadFiles(t *testing.T) {
 			}
 			title += fmt.Sprintf("%v", tc.names)
 
-			// Apply any necessary config changes
-			var restoreConfig func(a *app.App)
-			if tc.setupConfig != nil {
-				restoreConfig = tc.setupConfig(th.App)
-			}
-
 			t.Run(title, func(t *testing.T) {
-				if len(tc.openers) == 0 {
-					for _, name := range tc.names {
-						tc.openers = append(tc.openers, op(name))
+				// Apply any necessary config changes
+				if tc.setupConfig != nil {
+					restoreConfig := tc.setupConfig(th.App)
+					if restoreConfig != nil {
+						defer restoreConfig(th.App)
+					}
+				}
+
+				// Set the default values
+				client := th.Client
+				if tc.client != nil {
+					client = tc.client
+				}
+				channelId := channel.Id
+				if tc.channelId != "" {
+					channelId = tc.channelId
+				}
+
+				blobs := tc.blobs
+				if len(blobs) == 0 {
+					for _, name := range tc.names {
+						blobs = append(blobs, fileBytes(name))
+					}
+				}
+
+				var fileResp *model.FileUploadResponse
+				var resp *model.Response
+				if useMultipart {
+					fileResp, resp = testUploadFilesMultipart(t, client, channelId, tc.names, blobs, tc.clientIds)
+				} else {
+					fileResp, resp = testUploadFilesPost(t, client, channelId, tc.names, blobs, tc.clientIds, tc.useChunkedInSimplePost)
+				}
+
+				if tc.checkResponse != nil {
+					tc.checkResponse(t, resp)
+				} else {
+					if resp != nil {
+						require.Nil(t, resp.Error)
 					}
 				}
-				fileResp, resp := testUploadFiles(client, channelId, tc.names,
-					tc.openers, nil, tc.clientIds, useMultipart,
-					tc.useChunkedInSimplePost)
-				tc.checkResponse(t, resp)
 				if tc.skipSuccessValidation {
 					return
 				}
@@ -668,68 +561,63 @@ func TestUploadFiles(t *testing.T) {
 
 				for i, ri := range fileResp.FileInfos {
 					// The returned file info from the upload call will be missing some fields that will be stored in the database
-					checkEq(t, ri.CreatorId, tc.expectedCreatorId, "File should be assigned to user")
-					checkEq(t, ri.PostId, "", "File shouldn't have a post Id")
-					checkEq(t, ri.Path, "", "File path should not be set on returned info")
-					checkEq(t, ri.ThumbnailPath, "", "File thumbnail path should not be set on returned info")
-					checkEq(t, ri.PreviewPath, "", "File preview path should not be set on returned info")
+					assert.Equal(t, ri.CreatorId, tc.expectedCreatorId, "File should be assigned to user")
+					assert.Equal(t, ri.PostId, "", "File shouldn't have a post Id")
+					assert.Equal(t, ri.Path, "", "File path should not be set on returned info")
+					assert.Equal(t, ri.ThumbnailPath, "", "File thumbnail path should not be set on returned info")
+					assert.Equal(t, ri.PreviewPath, "", "File preview path should not be set on returned info")
 					if len(tc.clientIds) > i {
-						checkCond(t, len(fileResp.ClientIds) == len(tc.clientIds),
+						assert.True(t, len(fileResp.ClientIds) == len(tc.clientIds),
 							fmt.Sprintf("Wrong number of clientIds returned, expected %v, got %v", len(tc.clientIds), len(fileResp.ClientIds)))
-						checkEq(t, fileResp.ClientIds[i], tc.clientIds[i],
+						assert.Equal(t, fileResp.ClientIds[i], tc.clientIds[i],
 							fmt.Sprintf("Wrong clientId returned, expected %v, got %v", tc.clientIds[i], fileResp.ClientIds[i]))
 					}
 
 					dbInfo, err := th.App.Srv.Store.FileInfo().Get(ri.Id)
 					require.Nil(t, err)
-					checkEq(t, dbInfo.Id, ri.Id, "File id from response should match one stored in database")
-					checkEq(t, dbInfo.CreatorId, tc.expectedCreatorId, "F ile should be assigned to user")
-					checkEq(t, dbInfo.PostId, "", "File shouldn't have a post")
-					checkNeq(t, dbInfo.Path, "", "File path should be set in database")
+					assert.Equal(t, dbInfo.Id, ri.Id, "File id from response should match one stored in database")
+					assert.Equal(t, dbInfo.CreatorId, tc.expectedCreatorId, "F ile should be assigned to user")
+					assert.Equal(t, dbInfo.PostId, "", "File shouldn't have a post")
+					assert.NotEqual(t, dbInfo.Path, "", "File path should be set in database")
 					_, fname := filepath.Split(dbInfo.Path)
 					ext := filepath.Ext(fname)
 					name := fname[:len(fname)-len(ext)]
 					expectedDir := fmt.Sprintf("%v/teams/%v/channels/%v/users/%s/%s", date, FILE_TEAM_ID, channel.Id, ri.CreatorId, ri.Id)
 					expectedPath := fmt.Sprintf("%s/%s", expectedDir, fname)
-					checkEq(t, dbInfo.Path, expectedPath,
+					assert.Equal(t, dbInfo.Path, expectedPath,
 						fmt.Sprintf("File %v saved to:%q, expected:%q", dbInfo.Name, dbInfo.Path, expectedPath))
 
 					if tc.expectImage {
 						expectedThumbnailPath := fmt.Sprintf("%s/%s_thumb.jpg", expectedDir, name)
 						expectedPreviewPath := fmt.Sprintf("%s/%s_preview.jpg", expectedDir, name)
-						checkEq(t, dbInfo.ThumbnailPath, expectedThumbnailPath,
+						assert.Equal(t, dbInfo.ThumbnailPath, expectedThumbnailPath,
 							fmt.Sprintf("Thumbnail for %v saved to:%q, expected:%q", dbInfo.Name, dbInfo.ThumbnailPath, expectedThumbnailPath))
-						checkEq(t, dbInfo.PreviewPath, expectedPreviewPath,
+						assert.Equal(t, dbInfo.PreviewPath, expectedPreviewPath,
 							fmt.Sprintf("Preview for %v saved to:%q, expected:%q", dbInfo.Name, dbInfo.PreviewPath, expectedPreviewPath))
 
-						checkCond(t,
+						assert.True(t,
 							dbInfo.HasPreviewImage == tc.expectedImageHasPreview[i],
 							fmt.Sprintf("Image: HasPreviewImage should be set for %s", dbInfo.Name))
-						checkCond(t,
+						assert.True(t,
 							dbInfo.Width == tc.expectedImageWidths[i] && dbInfo.Height == tc.expectedImageHeights[i],
 							fmt.Sprintf("Image dimensions: expected %dwx%dh, got %vwx%dh",
 								tc.expectedImageWidths[i], tc.expectedImageHeights[i],
 								dbInfo.Width, dbInfo.Height))
 					}
 
-					/*if !tc.skipPayloadValidation {
+					if !tc.skipPayloadValidation {
 						compare := func(get func(string) ([]byte, *model.Response), name string) {
 							data, resp := get(ri.Id)
-							if resp.Error != nil {
-								t.Fatal(resp.Error)
-							}
+							require.NotNil(t, resp)
+							require.Nil(t, resp.Error)
 
 							expected, err := ioutil.ReadFile(filepath.Join(testDir, name))
-							if err != nil {
-								t.Fatal(err)
-							}
+							require.Nil(t, err)
 
 							if bytes.Compare(data, expected) != 0 {
 								tf, err := ioutil.TempFile("", fmt.Sprintf("test_%v_*_%s", i, name))
-								if err != nil {
-									t.Fatal(err)
-								}
-								io.Copy(tf, bytes.NewReader(data))
+								require.Nil(t, err)
+								_, _ = io.Copy(tf, bytes.NewReader(data))
 								tf.Close()
 								t.Errorf("Actual data mismatched %s, written to %q", name, tf.Name())
 							}
@@ -745,15 +633,12 @@ func TestUploadFiles(t *testing.T) {
 						if len(tc.expectedImageThumbnailNames) > i {
 							compare(client.GetFilePreview, tc.expectedImagePreviewNames[i])
 						}
-					}*/
+					}
 
 					th.cleanupTestFile(dbInfo)
 				}
 			})
 
-			if restoreConfig != nil {
-				restoreConfig(th.App)
-			}
 		}
 	}
 }

app/plugin_hooks_test.go

@@ -497,6 +497,7 @@ func TestHookFileWillBeUploaded(t *testing.T) {
 			package main
 
 			import (
+				"fmt"
 				"io"
 				"github.com/mattermost/mattermost-server/plugin"
 				"github.com/mattermost/mattermost-server/model"
@@ -507,7 +508,10 @@ func TestHookFileWillBeUploaded(t *testing.T) {
 			}
 
 			func (p *MyPlugin) FileWillBeUploaded(c *plugin.Context, info *model.FileInfo, file io.Reader, output io.Writer) (*model.FileInfo, string) {
-				output.Write([]byte("ignored"))
+				n, err := output.Write([]byte("ignored"))
+				if err != nil {
+					return info, fmt.Sprintf("FAILED to write output file n: %v, err: %v", n, err)
+				}
 				info.Name = "ignored"
 				return info, "rejected"
 			}
@@ -607,6 +611,7 @@ func TestHookFileWillBeUploaded(t *testing.T) {
 			import (
 				"io"
 				"bytes"
+				"fmt"
 				"github.com/mattermost/mattermost-server/plugin"
 				"github.com/mattermost/mattermost-server/model"
 			)
@@ -616,13 +621,17 @@ func TestHookFileWillBeUploaded(t *testing.T) {
 			}
 
 			func (p *MyPlugin) FileWillBeUploaded(c *plugin.Context, info *model.FileInfo, file io.Reader, output io.Writer) (*model.FileInfo, string) {
-				p.API.LogDebug(info.Name)
 				var buf bytes.Buffer
-				buf.ReadFrom(file)
-				p.API.LogDebug(buf.String())
+				n, err := buf.ReadFrom(file)
+				if err != nil {
+					return info, fmt.Sprintf("FAILED to read input file n: %v, err: %v", n, err)
+				}
 
 				outbuf := bytes.NewBufferString("changedtext")
-				io.Copy(output, outbuf)
+				n, err = io.Copy(output, outbuf)
+				if int(n) != len("changedtext") || err != nil {
+					return info, fmt.Sprintf("FAILED to write output file n: %v, err: %v", n, err)
+				}
 				info.Name = "modifiedinfo"
 				return info, ""
 			}