Fix some add errors not bubbling up

Author: code-asher

storage/local.go

@@ -55,18 +55,18 @@ func (s *Local) AddExtension(ctx context.Context, source string) (string, error)
 		return os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o644)
 	})
 	if err != nil {
-		return "", nil
+		return "", err
 	}
 
 	// Copy the VSIX itself as well.
 	vsixName := fmt.Sprintf("%s.%s-%s.vsix", identity.Publisher, identity.ID, identity.Version)
 	dst, err := os.OpenFile(filepath.Join(dir, vsixName), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o644)
 	if err != nil {
-		return "", nil
+		return "", err
 	}
 	_, err = io.Copy(dst, bytes.NewReader(vsixBytes))
 	if err != nil {
-		return "", nil
+		return "", err
 	}
 
 	return dir, nil

storage/storage_test.go

@@ -195,14 +195,11 @@ type file struct {
 	body []byte
 }
 
-func createVsix(manifest *storage.VSIXManifest) ([]byte, error) {
-	manifestBytes, err := xml.Marshal(manifest)
-	if err != nil {
-		return nil, err
-	}
-
+// createVSIX returns the bytes for a VSIX file containing the provided manifest
+// and an icon.
+func createVSIX(manifestBytes []byte) ([]byte, error) {
 	files := []file{{"icon.png", []byte("fake icon")}}
-	if manifest != nil {
+	if manifestBytes != nil {
 		files = append(files, file{"extension.vsixmanifest", manifestBytes})
 	}
 
@@ -223,6 +220,8 @@ func createVsix(manifest *storage.VSIXManifest) ([]byte, error) {
 	return buf.Bytes(), nil
 }
 
+// requireExtension verifies an extension exists in the extension directory and
+// that the received output matches the expected path.
 func requireExtension(t *testing.T, ext testutil.Extension, extdir string, got string) {
 	expected := filepath.Join(extdir, ext.Publisher, ext.Name, ext.LatestVersion)
 	require.Equal(t, expected, got)
@@ -238,85 +237,192 @@ func requireExtension(t *testing.T, ext testutil.Extension, extdir string, got s
 func TestAddExtension(t *testing.T) {
 	t.Parallel()
 
-	t.Run("HTTPOK", func(t *testing.T) {
-		t.Parallel()
-
-		ext := testutil.Extensions[0]
-		vsix, err := createVsix(testutil.ConvertExtensionToManifest(ext, ext.LatestVersion))
-		require.NoError(t, err)
-
-		server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
-			_, err := rw.Write(vsix)
-			require.NoError(t, err)
-		}))
-		defer server.Close()
+	ext := testutil.Extensions[0]
+	manifestBytes, err := xml.Marshal(testutil.ConvertExtensionToManifest(ext, ext.LatestVersion))
+	require.NoError(t, err)
+	vsix, err := createVSIX(manifestBytes)
+	require.NoError(t, err)
 
-		extdir := t.TempDir()
-		s := &storage.Local{ExtDir: extdir}
+	t.Run("HTTP", func(t *testing.T) {
+		t.Parallel()
 
-		got, err := s.AddExtension(context.Background(), server.URL)
-		require.NoError(t, err)
-		requireExtension(t, ext, extdir, got)
-	})
+		tests := []struct {
+			error   string
+			name    string
+			handler http.HandlerFunc
+			setup   func(extdir string) (string, error)
+		}{
+			{
+				name: "OK",
+			},
+			{
+				name:  "InternalError",
+				error: strconv.Itoa(http.StatusInternalServerError),
+				handler: func(rw http.ResponseWriter, r *http.Request) {
+					http.Error(rw, "something went wrong", http.StatusInternalServerError)
+				},
+			},
+			{
+				name:  "ReadError",
+				error: "unexpected EOF",
+				handler: func(rw http.ResponseWriter, r *http.Request) {
+					rw.Header().Set("Content-Length", "1")
+				},
+			},
+			{
+				name:  "InvalidZip",
+				error: "zip: not a valid zip file",
+				handler: func(rw http.ResponseWriter, r *http.Request) {
+					_, err := rw.Write([]byte{})
+					require.NoError(t, err)
+				},
+			},
+			{
+				name:  "ExtensionDirPerms",
+				error: "permission denied",
+				setup: func(extdir string) (string, error) {
+					// Disallow writing to the extension directory.
+					extdir = filepath.Join(extdir, "no-write")
+					return extdir, os.MkdirAll(extdir, 0o444)
+				},
+			},
+			{
+				name:  "CopyOverDirectory",
+				error: "is a directory",
+				setup: func(extdir string) (string, error) {
+					// Put a directory in the way of the vsix.
+					vsixName := fmt.Sprintf("%s.%s-%s.vsix", ext.Publisher, ext.Name, ext.LatestVersion)
+					vsixPath := filepath.Join(extdir, ext.Publisher, ext.Name, ext.LatestVersion, vsixName)
+					return extdir, os.MkdirAll(vsixPath, 0o755)
+				},
+			},
+		}
 
-	t.Run("HTTPError", func(t *testing.T) {
-		t.Parallel()
+		for _, test := range tests {
+			test := test
+			t.Run(test.name, func(t *testing.T) {
+				t.Parallel()
 
-		server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
-			http.Error(rw, "something went wrong", http.StatusInternalServerError)
-		}))
-		defer server.Close()
+				handler := test.handler
+				if handler == nil {
+					handler = func(rw http.ResponseWriter, r *http.Request) {
+						_, err := rw.Write(vsix)
+						require.NoError(t, err)
+					}
+				}
 
-		s := &storage.Local{ExtDir: t.TempDir()}
+				server := httptest.NewServer(http.HandlerFunc(handler))
+				defer server.Close()
 
-		_, err := s.AddExtension(context.Background(), server.URL)
-		require.Error(t, err)
+				extdir := t.TempDir()
+				if test.setup != nil {
+					extdir, err = test.setup(extdir)
+					require.NoError(t, err)
+				}
+				s := &storage.Local{ExtDir: extdir}
 
-		// The error should mention the status code.
-		require.Contains(t, err.Error(), strconv.Itoa(http.StatusInternalServerError))
+				got, err := s.AddExtension(context.Background(), server.URL)
+				if test.error != "" {
+					require.Error(t, err)
+					require.Regexp(t, test.error, err.Error())
+				} else {
+					requireExtension(t, ext, extdir, got)
+				}
+			})
+		}
 	})
 
-	t.Run("FileOK", func(t *testing.T) {
+	t.Run("File", func(t *testing.T) {
 		t.Parallel()
 
-		ext := testutil.Extensions[0]
-		vsix, err := createVsix(testutil.ConvertExtensionToManifest(ext, ext.LatestVersion))
-		require.NoError(t, err)
-
-		extdir := t.TempDir()
-		vsixPath := filepath.Join(extdir, "extension.vsix")
-		err = os.WriteFile(vsixPath, vsix, 0o644)
-		require.NoError(t, err)
+		tests := []struct {
+			error  string
+			name   string
+			source func(extdir string) (string, error)
+		}{
+			{
+				name: "OK",
+				source: func(extdir string) (string, error) {
+					vsixPath := filepath.Join(extdir, "extension.vsix")
+					return vsixPath, os.WriteFile(vsixPath, vsix, 0o644)
+				},
+			},
+			{
+				name:  "NotFound",
+				error: "foo\\.vsix.+no such file",
+				source: func(extdir string) (string, error) {
+					return filepath.Join(extdir, "foo.vsix"), nil
+				},
+			},
+			{
+				name:  "EmptyFile",
+				error: "zip: not a valid zip file",
+				source: func(extdir string) (string, error) {
+					vsixPath := filepath.Join(extdir, "foo.vsix")
+					return vsixPath, os.WriteFile(vsixPath, []byte{}, 0o644)
+				},
+			},
+			{
+				name:  "Unreadable",
+				error: "permission denied",
+				source: func(extdir string) (string, error) {
+					vsixPath := filepath.Join(extdir, "extension.vsix")
+					return vsixPath, os.WriteFile(vsixPath, []byte{}, 0o222)
+				},
+			},
+		}
 
-		s := &storage.Local{ExtDir: extdir}
+		for _, test := range tests {
+			test := test
+			t.Run(test.name, func(t *testing.T) {
+				t.Parallel()
 
-		got, err := s.AddExtension(context.Background(), vsixPath)
-		require.NoError(t, err)
-		requireExtension(t, ext, extdir, got)
-	})
+				extdir := t.TempDir()
+				s := &storage.Local{ExtDir: extdir}
 
-	t.Run("FileError", func(t *testing.T) {
-		t.Parallel()
+				source, err := test.source(extdir)
+				require.NoError(t, err)
 
-		extdir := t.TempDir()
-		s := &storage.Local{ExtDir: extdir}
-		_, err := s.AddExtension(context.Background(), filepath.Join(extdir, "foo.vsix"))
-		require.Error(t, err)
+				got, err := s.AddExtension(context.Background(), source)
+				if test.error != "" {
+					require.Error(t, err)
+					require.Regexp(t, test.error, err.Error())
+				} else {
+					requireExtension(t, ext, extdir, got)
+				}
+			})
+		}
 	})
 
-	t.Run("InvalidManifest", func(t *testing.T) {
+	t.Run("ValidateManifest", func(t *testing.T) {
 		t.Parallel()
 
 		tests := []struct {
-			error    string
-			manifest *storage.VSIXManifest
-			name     string
+			error         string
+			manifest      *storage.VSIXManifest
+			manifestBytes []byte
+			name          string
 		}{
 			{
 				error:    "not found",
 				name:     "Missing",
 				manifest: nil,
 			},
+			{
+				error:         "EOF",
+				name:          "Empty",
+				manifestBytes: []byte(""),
+			},
+			{
+				error:         "EOF",
+				name:          "TextFile",
+				manifestBytes: []byte("just some random text"),
+			},
+			{
+				error:         "XML syntax error",
+				name:          "SyntaxError",
+				manifestBytes: []byte("<PackageManifest/PackageManifest>"),
+			},
 			{
 				error:    "publisher",
 				name:     "MissingPublisher",
@@ -352,7 +458,12 @@ func TestAddExtension(t *testing.T) {
 			t.Run(test.name, func(t *testing.T) {
 				t.Parallel()
 
-				vsix, err := createVsix(test.manifest)
+				manifestBytes := test.manifestBytes
+				if manifestBytes == nil {
+					manifestBytes, err = xml.Marshal(test.manifest)
+					require.NoError(t, err)
+				}
+				vsix, err := createVSIX(manifestBytes)
 				require.NoError(t, err)
 
 				extdir := t.TempDir()
@@ -364,9 +475,7 @@ func TestAddExtension(t *testing.T) {
 
 				_, err = s.AddExtension(context.Background(), vsixPath)
 				require.Error(t, err)
-
-				// The error should mention what is wrong.
-				require.Contains(t, err.Error(), test.error)
+				require.Regexp(t, test.error, err.Error())
 			})
 		}
 	})