additional testing and fixes from code review

marginally more testing - stateGetter

increase coverage of integration test framework code

yes I know this is redundant
also add that to 'make test'

refactor to avoid viper calls at runtime

factor out 'fetchEditFiles' function

add unit test for app type determination code

remove unneeded 'fs.RemoveAll' from stategetter code

Makefile

@@ -213,6 +213,7 @@ deps:
 .state/fmt: $(SRC)
 	goimports -w pkg
 	goimports -w cmd
+	goimports -w integration
 	@mkdir -p .state
 	@touch .state/fmt
 
@@ -222,6 +223,7 @@ fmt: .state/build-deps .state/fmt
 .state/vet: $(SRC)
 	go vet ./pkg/...
 	go vet ./cmd/...
+	go vet ./integration/...
 	@mkdir -p .state
 	@touch .state/vet
 
@@ -230,6 +232,7 @@ vet: .state/vet
 .state/ineffassign: .state/build-deps $(SRC)
 	ineffassign ./pkg
 	ineffassign ./cmd
+	ineffassign ./integration
 	@mkdir -p .state
 	@touch .state/ineffassign
 
@@ -244,7 +247,7 @@ ineffassign: .state/ineffassign
 lint: vet ineffassign .state/lint
 
 .state/test: $(SRC)
-	go test ./pkg/... | grep -v '?'
+	go test ./pkg/... ./integration | grep -v '?'
 	@mkdir -p .state
 	@touch .state/test
 
@@ -260,7 +263,7 @@ race: lint .state/race
 .state/coverage.out: $(SRC)
 	@mkdir -p .state/
 	#the reduced parallelism here is to avoid hitting the memory limits - we consistently did so with two threads on a 4gb instance
-	go test -parallel 1 -p 1 -coverprofile=.state/coverage.out ./pkg/...
+	go test -parallel 1 -p 1 -coverprofile=.state/coverage.out ./pkg/... ./integration
 
 citest: .state/vet .state/ineffassign .state/lint .state/coverage.out
 

integration/lib.go

@@ -9,6 +9,7 @@ import (
 	"strings"
 
 	. "github.com/onsi/gomega"
+	"github.com/pkg/errors"
 	"github.com/pmezard/go-difflib/difflib"
 )
 
@@ -99,8 +100,11 @@ func CompareDir(expected, actual string, replacements map[string]string, ignored
 
 				fileIgnorePaths := ignoredKeys[relativeActualFilePath]
 
-				expectedContentsBytes = prettyAndCleanJSON(expectedContentsBytes, fileIgnorePaths)
-				actualContentsBytes = prettyAndCleanJSON(actualContentsBytes, fileIgnorePaths)
+				expectedContentsBytes, err = prettyAndCleanJSON(expectedContentsBytes, fileIgnorePaths)
+				Expect(err).NotTo(HaveOccurred())
+
+				actualContentsBytes, err = prettyAndCleanJSON(actualContentsBytes, fileIgnorePaths)
+				Expect(err).NotTo(HaveOccurred())
 			}
 
 			// kind of a hack -- remove any trailing newlines (because text editors are hard to use)
@@ -129,10 +133,12 @@ func CompareDir(expected, actual string, replacements map[string]string, ignored
 	return true, nil
 }
 
-func prettyAndCleanJSON(data []byte, keysToIgnore []string) []byte {
+func prettyAndCleanJSON(data []byte, keysToIgnore []string) ([]byte, error) {
 	var obj interface{}
 	err := json.Unmarshal(data, &obj)
-	Expect(err).NotTo(HaveOccurred())
+	if err != nil {
+		return nil, errors.Wrap(err, "unmarshal")
+	}
 
 	if _, ok := obj.(map[string]interface{}); ok && keysToIgnore != nil {
 		for _, key := range keysToIgnore {
@@ -141,9 +147,10 @@ func prettyAndCleanJSON(data []byte, keysToIgnore []string) []byte {
 	}
 
 	data, err = json.MarshalIndent(obj, "", "  ")
-	Expect(err).NotTo(HaveOccurred())
-
-	return data
+	if err != nil {
+		return nil, errors.Wrap(err, "marshal")
+	}
+	return data, nil
 }
 
 func replaceInJSON(obj map[string]interface{}, path string) map[string]interface{} {

integration/lib_test.go

@@ -1,6 +1,7 @@
 package integration
 
 import (
+	"encoding/json"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -60,6 +61,22 @@ func Test_replaceInJSON(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "remove nested nonexistent",
+			path: "b.c",
+			obj: map[string]interface{}{
+				"a": "b",
+				"abc": map[string]interface{}{
+					"bcd": "efg",
+				},
+			},
+			want: map[string]interface{}{
+				"a": "b",
+				"abc": map[string]interface{}{
+					"bcd": "efg",
+				},
+			},
+		},
 		{
 			name: "remove second level obj",
 			path: "abc.bcd",
@@ -131,3 +148,62 @@ func Test_replaceInJSON(t *testing.T) {
 		})
 	}
 }
+
+func Test_prettyAndCleanJSON(t *testing.T) {
+	tests := []struct {
+		name         string
+		data         interface{}
+		keysToIgnore []string
+		want         interface{}
+	}{
+		{
+			name: "basic",
+			data: map[string]interface{}{
+				"a": "b",
+				"abc": map[string]interface{}{
+					"hij": "klm",
+				},
+			},
+			keysToIgnore: []string{},
+			want: map[string]interface{}{
+				"a": "b",
+				"abc": map[string]interface{}{
+					"hij": "klm",
+				},
+			},
+		},
+		{
+			name: "remove two keys",
+			data: map[string]interface{}{
+				"a": "b",
+				"c": "d",
+				"abc": map[string]interface{}{
+					"hij": "klm",
+				},
+			},
+			keysToIgnore: []string{"c", "abc.d"},
+			want: map[string]interface{}{
+				"a": "b",
+				"abc": map[string]interface{}{
+					"hij": "klm",
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			req := require.New(t)
+			dataBytes, err := json.Marshal(tt.data)
+			req.NoError(err)
+
+			got, err := prettyAndCleanJSON(dataBytes, tt.keysToIgnore)
+			req.NoError(err)
+
+			var outData interface{}
+			err = json.Unmarshal(got, &outData)
+			req.NoError(err)
+
+			req.Equal(tt.want, outData)
+		})
+	}
+}

pkg/specs/apptype/determine_type.go

@@ -52,6 +52,7 @@ func NewInspector(
 		viper:  v,
 		state:  stateManager,
 		ui:     ui,
+		isEdit: v.GetBool("isEdit"),
 	}
 }
 
@@ -61,6 +62,7 @@ type inspector struct {
 	viper  *viper.Viper
 	state  state.Manager
 	ui     cli.Ui
+	isEdit bool
 }
 
 type FileFetcher interface {
@@ -81,23 +83,8 @@ func (i *inspector) DetermineApplicationType(ctx context.Context, upstream strin
 		return &localAppCopy{AppType: "runbook.replicated.app", LocalPath: parts[0]}, nil
 	}
 
-	if i.viper.GetBool("isEdit") {
-		state, err := i.state.TryLoad()
-		if err != nil {
-			return nil, errors.Wrap(err, "load app state")
-		}
-		upstreamContents := state.UpstreamContents()
-		if upstreamContents == nil {
-			return nil, fmt.Errorf("no upstream contents present")
-		}
-
-		if upstreamContents.AppRelease != nil {
-			return &localAppCopy{AppType: "replicated.app"}, nil
-		}
-
-		// create a new fetcher class that gets things from the state file
-		stateClient := stategetter.NewStateGetter(i.fs, i.logger, upstreamContents)
-		return i.determineTypeFromContents(ctx, upstream, stateClient)
+	if i.isEdit {
+		return i.fetchEditFiles(ctx)
 	}
 
 	i.ui.Info(fmt.Sprintf("Attempting to retrieve upstream %s ...", upstream))
@@ -125,6 +112,25 @@ func (i *inspector) DetermineApplicationType(ctx context.Context, upstream strin
 	return nil, errors.Errorf("upstream %s is not a replicated app, a github repo, or compatible with go-getter", upstream)
 }
 
+func (i *inspector) fetchEditFiles(ctx context.Context) (app LocalAppCopy, err error) {
+	state, err := i.state.TryLoad()
+	if err != nil {
+		return nil, errors.Wrap(err, "load app state")
+	}
+	upstreamContents := state.UpstreamContents()
+	if upstreamContents == nil {
+		return nil, fmt.Errorf("no upstream contents present")
+	}
+
+	if upstreamContents.AppRelease != nil {
+		return &localAppCopy{AppType: "replicated.app"}, nil
+	}
+
+	// create a new fetcher class that gets things from the state file
+	stateClient := stategetter.NewStateGetter(i.fs, i.logger, upstreamContents)
+	return i.determineTypeFromContents(ctx, "ship statefile", stateClient)
+}
+
 func (i *inspector) determineTypeFromContents(ctx context.Context, upstream string, fetcher FileFetcher) (app LocalAppCopy, err error) {
 	debug := level.Debug(i.logger)
 

pkg/specs/apptype/determine_type_test.go

@@ -0,0 +1,88 @@
+package apptype
+
+import (
+	"context"
+	"testing"
+
+	"github.com/replicatedhq/ship/pkg/state"
+	"github.com/replicatedhq/ship/pkg/testing/logger"
+	"github.com/spf13/afero"
+	"github.com/spf13/viper"
+	"github.com/stretchr/testify/require"
+)
+
+func Test_inspector_fetchEditFiles(t *testing.T) {
+	tests := []struct {
+		name    string
+		state   state.UpstreamContents
+		wantApp LocalAppCopy
+		wantErr bool
+	}{
+		{
+			name: "app release contents",
+			state: state.UpstreamContents{
+				AppRelease: &state.ShipRelease{},
+			},
+			wantApp: &localAppCopy{AppType: "replicated.app"},
+			wantErr: false,
+		},
+		{
+			name: "k8s yaml contents",
+			state: state.UpstreamContents{
+				UpstreamFiles: []state.UpstreamFile{
+					{FilePath: "test.yaml", FileContents: "YTogYg=="},
+				},
+			},
+			wantApp: &localAppCopy{AppType: "k8s"},
+			wantErr: false,
+		},
+		{
+			name: "helm chart yaml contents",
+			state: state.UpstreamContents{
+				UpstreamFiles: []state.UpstreamFile{
+					{FilePath: "test.yaml", FileContents: "YTogYg=="},
+					{FilePath: "Chart.yaml", FileContents: "VGhpcyBpcyBwcm9iYWJseSBub3QgYSB2YWxpZCBoZWxtIGNoYXJ0IHlhbWwgZmlsZQ=="},
+				},
+			},
+			wantApp: &localAppCopy{AppType: "helm"},
+			wantErr: false,
+		},
+		{
+			name: "inline replicated app contents",
+			state: state.UpstreamContents{
+				UpstreamFiles: []state.UpstreamFile{
+					{FilePath: "test.yaml", FileContents: "YTogYg=="},
+					{FilePath: "ship.yaml", FileContents: "SSBBTSBTSElQ"},
+				},
+			},
+			wantApp: &localAppCopy{AppType: "inline.replicated.app"},
+			wantErr: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			req := require.New(t)
+
+			fs := afero.Afero{Fs: afero.NewMemMapFs()}
+			tlog := logger.TestLogger{T: t}
+
+			vip := viper.New()
+			vip.Set("isEdit", true)
+
+			manager := state.NewManager(&tlog, fs, vip)
+			err := manager.SerializeUpstreamContents(&tt.state)
+			req.NoError(err)
+
+			i := NewInspector(&tlog, fs, vip, manager, nil).(*inspector)
+
+			gotApp, err := i.fetchEditFiles(context.Background())
+			if tt.wantErr {
+				req.Error(err)
+			} else {
+				req.NoError(err)
+			}
+
+			req.Equal(tt.wantApp.GetType(), gotApp.GetType())
+		})
+	}
+}

pkg/specs/chart.go

@@ -270,7 +270,7 @@ func (r *Resolver) resolveMetadata(ctx context.Context, upstream, localPath stri
 		return nil, errors.Wrap(err, "resolve base metadata")
 	}
 
-	if r.Viper.GetBool("isEdit") {
+	if r.isEdit {
 		debug.Log("event", "releaseNotes.resolve.cancel")
 		state, err := r.StateManager.TryLoad()
 		if err != nil {

pkg/specs/constructor.go

@@ -26,6 +26,7 @@ type Resolver struct {
 
 	Viper   *viper.Viper
 	NoOutro bool
+	isEdit  bool
 }
 
 // NewResolver builds a resolver from a Viper instance
@@ -55,5 +56,6 @@ func NewResolver(
 		AppResolver:   appresolver,
 		GitHubFetcher: github,
 		NoOutro:       v.GetBool("no-outro"),
+		isEdit:        v.GetBool("isEdit"),
 	}
 }

pkg/specs/interface.go

@@ -161,7 +161,7 @@ func (r *Resolver) ResolveRelease(ctx context.Context, upstream string) (*api.Re
 
 	debug.Log("event", "upstream.Serialize", "for", app.GetLocalPath(), "upstream", versionedUpstream)
 
-	if !r.Viper.GetBool("isEdit") {
+	if !r.isEdit {
 		err = r.StateManager.SerializeUpstream(versionedUpstream)
 		if err != nil {
 			return nil, errors.Wrapf(err, "write upstream")
@@ -213,7 +213,7 @@ func (r *Resolver) ResolveRelease(ctx context.Context, upstream string) (*api.Re
 		r.AppResolver.SetRunbook(app.GetLocalPath())
 		fallthrough
 	case "replicated.app":
-		if r.Viper.GetBool("isEdit") {
+		if r.isEdit {
 			return r.AppResolver.ResolveEditRelease(ctx)
 		}
 

pkg/specs/persist.go

@@ -12,7 +12,7 @@ import (
 )
 
 func (r *Resolver) persistToState(root string) error {
-	if r.Viper.GetBool("isEdit") {
+	if r.isEdit {
 		return nil
 	}