Update per review comments.

Signed-off-by: Natalie Arellano <narellano@vmware.com>

acceptance/acceptance_test.go

@@ -35,6 +35,7 @@ import (
 	"github.com/buildpacks/pack/internal/blob"
 	"github.com/buildpacks/pack/internal/builder"
 	"github.com/buildpacks/pack/internal/cache"
+	"github.com/buildpacks/pack/internal/logging"
 	"github.com/buildpacks/pack/internal/style"
 	h "github.com/buildpacks/pack/testhelpers"
 )
@@ -55,6 +56,15 @@ var (
 	suiteManager   *SuiteManager
 )
 
+type testWriter struct {
+	t *testing.T
+}
+
+func (w *testWriter) Write(p []byte) (n int, err error) {
+	w.t.Log(string(p))
+	return len(p), nil
+}
+
 func TestAcceptance(t *testing.T) {
 	var err error
 
@@ -67,7 +77,8 @@ func TestAcceptance(t *testing.T) {
 	registryConfig = h.RunRegistry(t)
 	defer registryConfig.StopRegistry(t)
 
-	inputPathsManager, err := NewInputPathsManager()
+	testWriter := testWriter{t}
+	inputPathsManager, err := NewInputPathsManager(logging.NewLogWithWriters(&testWriter, &testWriter))
 	h.AssertNil(t, err)
 
 	combos, err := getRunCombinations()

acceptance/github_asset_fetcher.go

@@ -17,24 +17,25 @@ import (
 	"strings"
 	"time"
 
+	"github.com/buildpacks/pack/logging"
+
 	"github.com/Masterminds/semver"
 
 	"github.com/google/go-github/v30/github"
 	"github.com/pkg/errors"
 
 	"github.com/buildpacks/pack/internal/blob"
-	"github.com/buildpacks/pack/internal/config"
-	ilogging "github.com/buildpacks/pack/internal/logging"
 )
 
 const (
-	assetCacheDir      = "test-assets-cache"
-	assetCacheManifest = "github.json"
+	assetCacheDir         = "test-assets-cache"
+	assetCacheManifest    = "github.json"
+	cacheManifestLifetime = 1 * time.Hour
 )
 
 type GithubAssetFetcher struct {
 	ctx          context.Context
-	logger       *ilogging.LogWithWriters
+	logger       logging.Logger
 	githubClient *github.Client
 	cacheDir     string
 }
@@ -49,27 +50,31 @@ type cachedAssets map[string][]string
 type cachedSources map[string]string
 type cachedVersions map[string]string
 
-func NewGithubAssetFetcher() (*GithubAssetFetcher, error) {
-	packHome, err := config.PackHome()
+func NewGithubAssetFetcher(logger logging.Logger) (*GithubAssetFetcher, error) {
+	relativeCacheDir := filepath.Join("..", "out", "tests", assetCacheDir)
+	cacheDir, err := filepath.Abs(relativeCacheDir)
 	if err != nil {
-		return nil, errors.Wrap(err, "getting pack home")
+		return nil, errors.Wrapf(err, "getting absolute path for %s", relativeCacheDir)
 	}
-	cacheDir := filepath.Join(packHome, assetCacheDir)
 	if err := os.MkdirAll(cacheDir, 0755); err != nil {
 		return nil, errors.Wrapf(err, "creating directory %s", cacheDir)
 	}
 
 	return &GithubAssetFetcher{
 		ctx:          context.TODO(),
-		logger:       ilogging.NewLogWithWriters(os.Stdout, os.Stderr), // TODO: not sure if this is what we really want
+		logger:       logger,
 		githubClient: github.NewClient(nil),
 		cacheDir:     cacheDir,
 	}, nil
 }
 
+// Fetch a GitHub release asset for the given repo that matches the regular expression.
+// The expression is something like 'pack-v\d+.\d+.\d+-macos'.
+// The asset may be found in the local cache or downloaded from GitHub.
+// The return value is the location of the asset on disk, or any error encountered.
 func (f *GithubAssetFetcher) FetchReleaseAsset(owner, repo, version string, expr *regexp.Regexp, extract bool) (string, error) {
 	if destPath, _ := f.cachedAsset(owner, repo, version, expr); destPath != "" {
-		fmt.Printf("found %s in cache for %s/%s %s\n", destPath, owner, repo, version)
+		f.logger.Infof("found %s in cache for %s/%s %s\n", destPath, owner, repo, version)
 		return destPath, nil
 	}
 
@@ -144,7 +149,7 @@ func extractType(extract bool, assetName string) string {
 
 func (f *GithubAssetFetcher) FetchReleaseSource(owner, repo, version string) (string, error) {
 	if destDir, _ := f.cachedSource(owner, repo, version); destDir != "" {
-		fmt.Printf("found %s in cache for %s/%s %s\n", destDir, owner, repo, version)
+		f.logger.Infof("found %s in cache for %s/%s %s\n", destDir, owner, repo, version)
 		return destDir, nil
 	}
 
@@ -171,9 +176,12 @@ func (f *GithubAssetFetcher) FetchReleaseSource(owner, repo, version string) (st
 	return destDir, nil
 }
 
+// Fetch a GitHub release version that is n minor versions older than the latest.
+// Ex: when n is 0, the latest release version is returned.
+// Ex: when n is -1, the latest patch of the previous minor version is returned.
 func (f *GithubAssetFetcher) FetchReleaseVersion(owner, repo string, n int) (string, error) {
 	if version, _ := f.cachedVersion(owner, repo, n); version != "" {
-		fmt.Printf("found %s in cache for %s/%s %d\n", version, owner, repo, n)
+		f.logger.Infof("found %s in cache for %s/%s %d\n", version, owner, repo, n)
 		return version, nil
 	}
 
@@ -182,6 +190,9 @@ func (f *GithubAssetFetcher) FetchReleaseVersion(owner, repo string, n int) (str
 	if err != nil {
 		return "", errors.Wrap(err, "listing releases")
 	}
+	if len(releases) == 0 {
+		return "", fmt.Errorf("no releases found for %s/%s", owner, repo)
+	}
 
 	// sort all release versions
 	versions := make([]*semver.Version, len(releases))
@@ -274,7 +285,7 @@ func (f *GithubAssetFetcher) loadCacheManifest() (assetCache, error) {
 	}
 
 	// invalidate cache manifest that is too old
-	if time.Since(cacheManifest.ModTime()) > 1*time.Hour {
+	if time.Since(cacheManifest.ModTime()) > cacheManifestLifetime {
 		return assetCache{}, nil
 	}
 
@@ -316,7 +327,6 @@ func (f *GithubAssetFetcher) writeCacheManifest(owner, repo string, op func(cach
 	if err != nil {
 		return errors.Wrap(err, "marshaling cache manifest content")
 	}
-	content = append(content, "\n"...)
 
 	return ioutil.WriteFile(filepath.Join(f.cacheDir, assetCacheManifest), content, 0644)
 }

acceptance/input_paths_manager.go

@@ -1,13 +1,14 @@
 package acceptance
 
 import (
-	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
 	"regexp"
 	"runtime"
 
+	"github.com/buildpacks/pack/logging"
+
 	"github.com/pkg/errors"
 
 	"github.com/buildpacks/pack/internal/style"
@@ -27,9 +28,10 @@ type InputPathsManager struct {
 	previousPackFixturesPath string
 	lifecyclePath            string
 	previousLifecyclePath    string
+	logger                   logging.Logger
 }
 
-func NewInputPathsManager() (*InputPathsManager, error) {
+func NewInputPathsManager(logger logging.Logger) (*InputPathsManager, error) {
 	packPath := os.Getenv(envPackPath)
 	previousPackPath := os.Getenv(envPreviousPackPath)
 	previousPackFixturesPath := os.Getenv(envPreviousPackFixturesPath)
@@ -61,17 +63,18 @@ func NewInputPathsManager() (*InputPathsManager, error) {
 		previousPackFixturesPath: inputPaths[2],
 		lifecyclePath:            inputPaths[3],
 		previousLifecyclePath:    inputPaths[4],
+		logger:                   logger,
 	}, nil
 }
 
 func (m *InputPathsManager) FillInRequiredPaths(c runCombo) error {
-	githubAssetFetcher, err := NewGithubAssetFetcher()
+	githubAssetFetcher, err := NewGithubAssetFetcher(m.logger)
 	if err != nil {
 		return errors.Wrap(err, "initializing GitHub asset fetcher")
 	}
 
 	if (c.Pack == "previous" || c.PackCreateBuilder == "previous") && m.previousPackPath == "" {
-		fmt.Printf("run combination %+v requires %s to be set\n", c, style.Symbol(envPreviousPackPath))
+		m.logger.Infof("run combination %+v requires %s to be set\n", c, style.Symbol(envPreviousPackPath))
 
 		version, err := githubAssetFetcher.FetchReleaseVersion("buildpacks", "pack", 0)
 		if err != nil {
@@ -90,11 +93,11 @@ func (m *InputPathsManager) FillInRequiredPaths(c runCombo) error {
 		}
 		assetPath := filepath.Join(assetDir, packBinaryName())
 
-		fmt.Printf("using %s for previous pack path\n", assetPath)
+		m.logger.Infof("using %s for previous pack path\n", assetPath)
 		m.previousPackPath = assetPath
 	}
 	if (c.Pack == "previous" || c.PackCreateBuilder == "previous") && m.previousPackFixturesPath == "" {
-		fmt.Printf("run combination %+v requires %s to be set\n", c, style.Symbol(envPreviousPackFixturesPath))
+		m.logger.Infof("run combination %+v requires %s to be set\n", c, style.Symbol(envPreviousPackFixturesPath))
 
 		version, err := githubAssetFetcher.FetchReleaseVersion("buildpacks", "pack", 0)
 		if err != nil {
@@ -110,14 +113,15 @@ func (m *InputPathsManager) FillInRequiredPaths(c runCombo) error {
 		if err != nil {
 			return errors.Wrapf(err, "reading directory %s", sourceDir)
 		}
+		// GitHub source tarballs have a top-level directory whose name includes the current commit sha.
 		innerDir := fis[0].Name()
 		fixturesDir := filepath.Join(sourceDir, innerDir, "acceptance", "testdata", "pack_fixtures")
 
-		fmt.Printf("using %s for previous pack fixtures path\n", fixturesDir)
+		m.logger.Infof("using %s for previous pack fixtures path\n", fixturesDir)
 		m.previousPackFixturesPath = fixturesDir
 	}
 	if c.Lifecycle == "current" && m.lifecyclePath == "" {
-		fmt.Printf("run combination %+v requires %s to be set\n", c, style.Symbol(envLifecyclePath))
+		m.logger.Infof("run combination %+v requires %s to be set\n", c, style.Symbol(envLifecyclePath))
 
 		version, err := githubAssetFetcher.FetchReleaseVersion("buildpacks", "lifecycle", 0)
 		if err != nil {
@@ -135,11 +139,11 @@ func (m *InputPathsManager) FillInRequiredPaths(c runCombo) error {
 			return errors.Wrap(err, "fetching release asset")
 		}
 
-		fmt.Printf("using %s for lifecycle path\n", assetPath)
+		m.logger.Infof("using %s for lifecycle path\n", assetPath)
 		m.lifecyclePath = assetPath
 	}
 	if c.Lifecycle == "previous" && m.previousLifecyclePath == "" {
-		fmt.Printf("run combination %+v requires %s to be set\n", c, style.Symbol(envPreviousLifecyclePath))
+		m.logger.Infof("run combination %+v requires %s to be set\n", c, style.Symbol(envPreviousLifecyclePath))
 
 		version, err := githubAssetFetcher.FetchReleaseVersion("buildpacks", "lifecycle", -1)
 		if err != nil {
@@ -157,7 +161,7 @@ func (m *InputPathsManager) FillInRequiredPaths(c runCombo) error {
 			return errors.Wrap(err, "fetching release asset")
 		}
 
-		fmt.Printf("using %s for previous lifecycle path\n", assetPath)
+		m.logger.Infof("using %s for previous lifecycle path\n", assetPath)
 		m.previousLifecyclePath = assetPath
 	}
 	return nil

acceptance/run_combination.go

@@ -20,6 +20,9 @@ type runCombo struct {
 	Lifecycle         string `json:"lifecycle"`
 }
 
+//nolint:unused // TODO: nolint directives in this file shouldn't be necessary, as
+// all of the code is used. However lint errors are returned without these directives.
+// Possibly related to https://github.com/golangci/golangci-lint/issues/791.
 type resolvedRunCombo struct {
 	packCreateBuilderFixturesDir string
 	packFixturesDir              string
@@ -33,6 +36,7 @@ func (c *runCombo) String() string {
 	return fmt.Sprintf("p_%s cb_%s lc_%s", c.Pack, c.PackCreateBuilder, c.Lifecycle)
 }
 
+//nolint
 func getRunCombinations() ([]runCombo, error) {
 	combos := []runCombo{
 		{Pack: "current", PackCreateBuilder: "current", Lifecycle: "default"}, // TODO: the current behavior for
@@ -48,6 +52,7 @@ func getRunCombinations() ([]runCombo, error) {
 	return parseSuiteConfig(suiteConfig)
 }
 
+//nolint:unused
 func parseSuiteConfig(config string) ([]runCombo, error) {
 	var cfgs []runCombo
 	if err := json.Unmarshal([]byte(config), &cfgs); err != nil {
@@ -80,6 +85,7 @@ func parseSuiteConfig(config string) ([]runCombo, error) {
 	return cfgs, nil
 }
 
+//nolint
 func resolveRunCombinations(
 	combos []runCombo,
 	packPath string,