From 226f6dc18cf348bfc138834c2e86402572ea361c Mon Sep 17 00:00:00 2001 From: Chris Olszewski Date: Fri, 3 Feb 2023 09:39:34 -0800 Subject: [PATCH] feat: Use lockfiles when determining what packages changed across commits (#3250) This PR attempts to address #2162 This PR was written to be conservative by nature and it will mark packages as changed on almost any lockfile change that isn't just a version bump. Notable changes: - Extended SCM interface as we need to be able to reconstruct old lockfile - Refactored calculation of the transitive closure of the package graph so a full `Context` object isn't required - Merged `TransitiveDeps` and `ExternalDeps` as they contained almost identical information I haven't been able to fully test this, but from a quick spot check of pnpm it seems to behave as expected. Needed testing: - [x] Npm - [x] Yarn - [x] Yarn2+ - [x] Pnpm --- .../lockfile_aware_caching/berry.t | 17 ++ .../lockfile_aware_caching/npm.t | 17 ++ .../lockfile_aware_caching/pnpm.t | 17 ++ .../lockfile_aware_caching/setup.sh | 2 + .../lockfile_aware_caching/yarn.t | 17 ++ cli/internal/context/context.go | 135 ++++++++---- cli/internal/fs/package_json.go | 4 +- cli/internal/lockfile/berry_lockfile.go | 12 ++ cli/internal/lockfile/lockfile.go | 27 +++ cli/internal/lockfile/npm_lockfile.go | 9 + cli/internal/lockfile/pnpm_lockfile.go | 12 ++ cli/internal/lockfile/pnpm_lockfile_test.go | 6 +- cli/internal/lockfile/yarn_lockfile.go | 7 + cli/internal/packagemanager/berry.go | 2 +- cli/internal/packagemanager/npm.go | 2 +- cli/internal/packagemanager/packagemanager.go | 7 +- cli/internal/packagemanager/pnpm.go | 2 +- cli/internal/packagemanager/pnpm6.go | 2 +- cli/internal/packagemanager/yarn.go | 2 +- cli/internal/prune/prune.go | 11 +- cli/internal/scm/git.go | 13 ++ cli/internal/scm/scm.go | 2 + cli/internal/scm/stub.go | 4 + cli/internal/scope/scope.go | 81 ++++++-- cli/internal/scope/scope_test.go | 193 +++++++++++++++++- 25 files changed, 523 insertions(+), 80 deletions(-) diff --git a/cli/integration_tests/lockfile_aware_caching/berry.t b/cli/integration_tests/lockfile_aware_caching/berry.t index befe3e7fc8e792..5d7a013d582c1f 100644 --- a/cli/integration_tests/lockfile_aware_caching/berry.t +++ b/cli/integration_tests/lockfile_aware_caching/berry.t @@ -53,6 +53,14 @@ Only b should have a cache miss Cached: 0 cached, 1 total Time:\s*[\.0-9]+m?s (re) +Add lockfile changes to a commit + $ git add . && git commit -m "bump lockfile" --quiet +Only root and b should be rebuilt since only the deps for b had a version bump + $ ${TURBO} build --filter="[HEAD^1]" --dry=json | jq ".packages" + [ + "//", + "b" + ] Bump of root workspace invalidates all packages $ patch yarn.lock turbo-bump.patch @@ -79,3 +87,12 @@ Bump of root workspace invalidates all packages Cached: 0 cached, 1 total Time:\s*[\.0-9]+m?s (re) +Add lockfile changes to a commit + $ git add . && git commit -m "global lockfile change" --quiet +Everything should be rebuilt as a dependency of the root package got bumped + $ ${TURBO} build --filter="[HEAD^1]" --dry=json | jq ".packages | sort" + [ + "//", + "a", + "b" + ] diff --git a/cli/integration_tests/lockfile_aware_caching/npm.t b/cli/integration_tests/lockfile_aware_caching/npm.t index a37c502cca73b3..d146e5d71b5858 100644 --- a/cli/integration_tests/lockfile_aware_caching/npm.t +++ b/cli/integration_tests/lockfile_aware_caching/npm.t @@ -69,6 +69,14 @@ Only b should have a cache miss Cached: 0 cached, 1 total Time:\s*[\.0-9]+m?s (re) +Add lockfile changes to a commit + $ git add . && git commit -m "bump lockfile" --quiet +Only root and b should be rebuilt since only the deps for b had a version bump + $ ${TURBO} build --filter="[HEAD^1]" --dry=json | jq ".packages" + [ + "//", + "b" + ] Bump of root workspace invalidates all packages $ patch package-lock.json turbo-bump.patch @@ -103,3 +111,12 @@ Bump of root workspace invalidates all packages Cached: 0 cached, 1 total Time:\s*[\.0-9]+m?s (re) +Add lockfile changes to a commit + $ git add . && git commit -m "global lockfile change" --quiet +Everything should be rebuilt as a dependency of the root package got bumped + $ ${TURBO} build --filter="[HEAD^1]" --dry=json | jq ".packages | sort" + [ + "//", + "a", + "b" + ] diff --git a/cli/integration_tests/lockfile_aware_caching/pnpm.t b/cli/integration_tests/lockfile_aware_caching/pnpm.t index b0dcff3654de03..bcdb8116989cfc 100644 --- a/cli/integration_tests/lockfile_aware_caching/pnpm.t +++ b/cli/integration_tests/lockfile_aware_caching/pnpm.t @@ -69,6 +69,14 @@ Only b should have a cache miss Cached: 0 cached, 1 total Time:\s*[\.0-9]+m?s (re) +Add lockfile changes to a commit + $ git add . && git commit -m "bump pnpm-lock" --quiet +Only root and b should be rebuilt since only the deps for b had a version bump + $ ${TURBO} build --filter="[HEAD^1]" --dry=json | jq ".packages" + [ + "//", + "b" + ] Bump of root workspace invalidates all packages $ patch pnpm-lock.yaml turbo-bump.patch @@ -103,3 +111,12 @@ Bump of root workspace invalidates all packages Cached: 0 cached, 1 total Time:\s*[\.0-9]+m?s (re) +Add lockfile changes to a commit + $ git add . && git commit -m "global lockfile change" --quiet +Everything should be rebuilt as a dependency of the root package got bumped + $ ${TURBO} build --filter="[HEAD^1]" --dry=json | jq ".packages | sort" + [ + "//", + "a", + "b" + ] diff --git a/cli/integration_tests/lockfile_aware_caching/setup.sh b/cli/integration_tests/lockfile_aware_caching/setup.sh index 0bd5264de1dc37..d4fc8df8343a79 100755 --- a/cli/integration_tests/lockfile_aware_caching/setup.sh +++ b/cli/integration_tests/lockfile_aware_caching/setup.sh @@ -10,6 +10,8 @@ git init ${TARGET_DIR} --quiet GIT_ARGS="--git-dir=${TARGET_DIR}/.git --work-tree=${TARGET_DIR}" git ${GIT_ARGS} config user.email "turbo-test@example.com" git ${GIT_ARGS} config user.name "Turbo Test" +echo ".turbo" >> ${TARGET_DIR}/.gitignore +echo "node_modules" >> ${TARGET_DIR}/.gitignore git ${GIT_ARGS} add . git ${GIT_ARGS} commit -m "Initial" --quiet diff --git a/cli/integration_tests/lockfile_aware_caching/yarn.t b/cli/integration_tests/lockfile_aware_caching/yarn.t index 6b5762f4935a98..6db3a08a1f63a3 100644 --- a/cli/integration_tests/lockfile_aware_caching/yarn.t +++ b/cli/integration_tests/lockfile_aware_caching/yarn.t @@ -69,6 +69,14 @@ Only b should have a cache miss Cached: 0 cached, 1 total Time:\s*[\.0-9]+m?s (re) +Add lockfile changes to a commit + $ git add . && git commit -m "bump lockfile" --quiet +Only root and b should be rebuilt since only the deps for b had a version bump + $ ${TURBO} build --filter="[HEAD^1]" --dry=json | jq ".packages" + [ + "//", + "b" + ] Bump of root workspace invalidates all packages $ patch yarn.lock turbo-bump.patch @@ -103,3 +111,12 @@ Bump of root workspace invalidates all packages Cached: 0 cached, 1 total Time:\s*[\.0-9]+m?s (re) +Add lockfile changes to a commit + $ git add . && git commit -m "global lockfile change" --quiet +Everything should be rebuilt as a dependency of the root package got bumped + $ ${TURBO} build --filter="[HEAD^1]" --dry=json | jq ".packages | sort" + [ + "//", + "a", + "b" + ] diff --git a/cli/internal/context/context.go b/cli/internal/context/context.go index c87e5497d07480..aa56a2d7113825 100644 --- a/cli/internal/context/context.go +++ b/cli/internal/context/context.go @@ -230,10 +230,7 @@ func BuildPackageGraph(repoRoot turbopath.AbsoluteSystemPath, rootPackageJSON *f } func (c *Context) resolveWorkspaceRootDeps(rootPackageJSON *fs.PackageJSON, warnings *Warnings) error { - seen := mapset.NewSet() - var lockfileEg errgroup.Group pkg := rootPackageJSON - depSet := mapset.NewSet() pkg.UnresolvedExternalDeps = make(map[string]string) for dep, version := range pkg.DevDependencies { pkg.UnresolvedExternalDeps[dep] = version @@ -245,25 +242,25 @@ func (c *Context) resolveWorkspaceRootDeps(rootPackageJSON *fs.PackageJSON, warn pkg.UnresolvedExternalDeps[dep] = version } if c.Lockfile != nil { - pkg.TransitiveDeps = []string{} - c.resolveDepGraph(&lockfileEg, pkg, pkg.UnresolvedExternalDeps, depSet, seen, pkg) - if err := lockfileEg.Wait(); err != nil { + depSet, err := TransitiveClosure(pkg, c.Lockfile) + if err != nil { warnings.append(err) // Return early to skip using results of incomplete dep graph resolution return nil } - pkg.ExternalDeps = make([]string, 0, depSet.Cardinality()) + pkg.TransitiveDeps = make([]lockfile.Package, 0, depSet.Cardinality()) for _, v := range depSet.ToSlice() { - pkg.ExternalDeps = append(pkg.ExternalDeps, fmt.Sprintf("%v", v)) + dep := v.(lockfile.Package) + pkg.TransitiveDeps = append(pkg.TransitiveDeps, dep) } - sort.Strings(pkg.ExternalDeps) - hashOfExternalDeps, err := fs.HashObject(pkg.ExternalDeps) + sort.Sort(lockfile.ByKey(pkg.TransitiveDeps)) + hashOfExternalDeps, err := fs.HashObject(pkg.TransitiveDeps) if err != nil { return err } pkg.ExternalDepsHash = hashOfExternalDeps } else { - pkg.ExternalDeps = []string{} + pkg.TransitiveDeps = []lockfile.Package{} pkg.ExternalDepsHash = "" } @@ -280,7 +277,6 @@ func (c *Context) populateWorkspaceGraphForPackageJSON(pkg *fs.PackageJSON, root depMap := make(map[string]string) internalDepsSet := make(dag.Set) externalUnresolvedDepsSet := make(dag.Set) - externalDepSet := mapset.NewSet() pkg.UnresolvedExternalDeps = make(map[string]string) for dep, version := range pkg.DevDependencies { @@ -320,31 +316,29 @@ func (c *Context) populateWorkspaceGraphForPackageJSON(pkg *fs.PackageJSON, root } } - pkg.TransitiveDeps = []string{} - seen := mapset.NewSet() - lockfileEg := &errgroup.Group{} - c.resolveDepGraph(lockfileEg, pkg, pkg.UnresolvedExternalDeps, externalDepSet, seen, pkg) - if err := lockfileEg.Wait(); err != nil { + externalDeps, err := TransitiveClosure(pkg, c.Lockfile) + if err != nil { warnings.append(err) // reset external deps to original state - externalDepSet = mapset.NewSet() + externalDeps = mapset.NewSet() } // when there are no internal dependencies, we need to still add these leafs to the graph if internalDepsSet.Len() == 0 { c.WorkspaceGraph.Connect(dag.BasicEdge(pkg.Name, core.ROOT_NODE_NAME)) } - pkg.ExternalDeps = make([]string, 0, externalDepSet.Cardinality()) - for _, v := range externalDepSet.ToSlice() { - pkg.ExternalDeps = append(pkg.ExternalDeps, fmt.Sprintf("%v", v)) + pkg.TransitiveDeps = make([]lockfile.Package, 0, externalDeps.Cardinality()) + for _, dependency := range externalDeps.ToSlice() { + dependency := dependency.(lockfile.Package) + pkg.TransitiveDeps = append(pkg.TransitiveDeps, dependency) } pkg.InternalDeps = make([]string, 0, internalDepsSet.Len()) for _, v := range internalDepsSet.List() { pkg.InternalDeps = append(pkg.InternalDeps, fmt.Sprintf("%v", v)) } sort.Strings(pkg.InternalDeps) - sort.Strings(pkg.ExternalDeps) - hashOfExternalDeps, err := fs.HashObject(pkg.ExternalDeps) + sort.Sort(lockfile.ByKey(pkg.TransitiveDeps)) + hashOfExternalDeps, err := fs.HashObject(pkg.TransitiveDeps) if err != nil { return err } @@ -374,40 +368,50 @@ func (c *Context) parsePackageJSON(repoRoot turbopath.AbsoluteSystemPath, pkgJSO return nil } -func (c *Context) resolveDepGraph(wg *errgroup.Group, workspace *fs.PackageJSON, unresolvedDirectDeps map[string]string, resolvedDepsSet mapset.Set, seen mapset.Set, pkg *fs.PackageJSON) { - if c.Lockfile == (lockfile.Lockfile)(nil) { - return +// TransitiveClosure the set of all lockfile keys that pkg depends on +func TransitiveClosure(pkg *fs.PackageJSON, lockFile lockfile.Lockfile) (mapset.Set, error) { + if lockfile.IsNil(lockFile) { + return nil, fmt.Errorf("No lockfile available to do analysis on") + } + + resolvedPkgs := mapset.NewSet() + lockfileEg := &errgroup.Group{} + + transitiveClosureHelper(lockfileEg, pkg, lockFile, pkg.UnresolvedExternalDeps, resolvedPkgs) + + if err := lockfileEg.Wait(); err != nil { + return nil, err } + + return resolvedPkgs, nil +} + +func transitiveClosureHelper(wg *errgroup.Group, pkg *fs.PackageJSON, lockfile lockfile.Lockfile, unresolvedDirectDeps map[string]string, resolvedDeps mapset.Set) { for directDepName, unresolvedVersion := range unresolvedDirectDeps { directDepName := directDepName unresolvedVersion := unresolvedVersion wg.Go(func() error { - lockfilePkg, err := c.Lockfile.ResolvePackage(workspace.Dir.ToUnixPath(), directDepName, unresolvedVersion) + lockfilePkg, err := lockfile.ResolvePackage(pkg.Dir.ToUnixPath(), directDepName, unresolvedVersion) if err != nil { return err } - if !lockfilePkg.Found || seen.Contains(lockfilePkg.Key) { + if !lockfilePkg.Found || resolvedDeps.Contains(lockfilePkg) { return nil } - seen.Add(lockfilePkg.Key) - - pkg.Mu.Lock() - pkg.TransitiveDeps = append(pkg.TransitiveDeps, lockfilePkg.Key) - pkg.Mu.Unlock() - resolvedDepsSet.Add(fmt.Sprintf("%s@%s", lockfilePkg.Key, lockfilePkg.Version)) + resolvedDeps.Add(lockfilePkg) - allDeps, ok := c.Lockfile.AllDependencies(lockfilePkg.Key) + allDeps, ok := lockfile.AllDependencies(lockfilePkg.Key) if !ok { panic(fmt.Sprintf("Unable to find entry for %s", lockfilePkg.Key)) } if len(allDeps) > 0 { - c.resolveDepGraph(wg, workspace, allDeps, resolvedDepsSet, seen, pkg) + transitiveClosureHelper(wg, pkg, lockfile, allDeps, resolvedDeps) } return nil @@ -443,3 +447,62 @@ func (c *Context) InternalDependencies(start []string) ([]string, error) { return targets, nil } + +// ChangedPackages returns a list of changed packages based on the contents of a previous lockfile +// This assumes that none of the package.json in the workspace change, it is +// the responsibility of the caller to verify this. +func (c *Context) ChangedPackages(previousLockfile lockfile.Lockfile) ([]string, error) { + if lockfile.IsNil(previousLockfile) || lockfile.IsNil(c.Lockfile) { + return nil, fmt.Errorf("Cannot detect changed packages without previous and current lockfile") + } + + didPackageChange := func(pkgName string, pkg *fs.PackageJSON) bool { + previousDeps, err := TransitiveClosure(pkg, previousLockfile) + if err != nil || previousDeps.Cardinality() != len(pkg.TransitiveDeps) { + return true + } + + prevExternalDeps := make([]lockfile.Package, 0, previousDeps.Cardinality()) + for _, d := range previousDeps.ToSlice() { + prevExternalDeps = append(prevExternalDeps, d.(lockfile.Package)) + } + sort.Sort(lockfile.ByKey(prevExternalDeps)) + + for i := range prevExternalDeps { + if prevExternalDeps[i] != pkg.TransitiveDeps[i] { + return true + } + } + return false + } + + changedPkgs := make([]string, 0, len(c.WorkspaceInfos)) + + // check if prev and current have "global" changes e.g. lockfile bump + globalChange := c.Lockfile.GlobalChange(previousLockfile) + + for pkgName, pkg := range c.WorkspaceInfos { + if globalChange { + break + } + if didPackageChange(pkgName, pkg) { + if pkgName == util.RootPkgName { + globalChange = true + } else { + changedPkgs = append(changedPkgs, pkgName) + } + } + } + + if globalChange { + changedPkgs = make([]string, 0, len(c.WorkspaceInfos)) + for pkgName := range c.WorkspaceInfos { + changedPkgs = append(changedPkgs, pkgName) + } + sort.Strings(changedPkgs) + return changedPkgs, nil + } + + sort.Strings(changedPkgs) + return changedPkgs, nil +} diff --git a/cli/internal/fs/package_json.go b/cli/internal/fs/package_json.go index 696c680900cfad..4aa2b2b32703b2 100644 --- a/cli/internal/fs/package_json.go +++ b/cli/internal/fs/package_json.go @@ -6,6 +6,7 @@ import ( "path/filepath" "sync" + "github.com/vercel/turbo/cli/internal/lockfile" "github.com/vercel/turbo/cli/internal/turbopath" ) @@ -32,8 +33,7 @@ type PackageJSON struct { Dir turbopath.AnchoredSystemPath `json:"-"` InternalDeps []string `json:"-"` UnresolvedExternalDeps map[string]string `json:"-"` - ExternalDeps []string `json:"-"` - TransitiveDeps []string `json:"-"` + TransitiveDeps []lockfile.Package `json:"-"` LegacyTurboConfig *TurboJSON `json:"turbo"` Mu sync.Mutex `json:"-"` ExternalDepsHash string `json:"-"` diff --git a/cli/internal/lockfile/berry_lockfile.go b/cli/internal/lockfile/berry_lockfile.go index 3571b2ccf4edca..e76f230d7209ea 100644 --- a/cli/internal/lockfile/berry_lockfile.go +++ b/cli/internal/lockfile/berry_lockfile.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "io" + "reflect" "regexp" "sort" "strconv" @@ -400,6 +401,17 @@ func DecodeBerryLockfile(contents []byte) (*BerryLockfile, error) { return &lockfile, nil } +// GlobalChange checks if there are any differences between lockfiles that would completely invalidate +// the cache. +func (l *BerryLockfile) GlobalChange(other Lockfile) bool { + otherBerry, ok := other.(*BerryLockfile) + return !ok || + l.cacheKey != otherBerry.cacheKey || + l.version != otherBerry.version || + // This is probably overly cautious, but getting it correct will be hard + !reflect.DeepEqual(l.patches, otherBerry.patches) +} + // Fields shared between _Locator and _Descriptor type _Ident struct { // Scope of package without leading @ diff --git a/cli/internal/lockfile/lockfile.go b/cli/internal/lockfile/lockfile.go index c4eabd2401c3f4..371d7eb57f5c4d 100644 --- a/cli/internal/lockfile/lockfile.go +++ b/cli/internal/lockfile/lockfile.go @@ -3,6 +3,8 @@ package lockfile import ( "io" + "reflect" + "sort" "github.com/vercel/turbo/cli/internal/turbopath" ) @@ -19,6 +21,14 @@ type Lockfile interface { Encode(w io.Writer) error // Patches return a list of patches used in the lockfile Patches() []turbopath.AnchoredUnixPath + // GlobalChange checks if there are any differences between lockfiles that would completely invalidate + // the cache. + GlobalChange(other Lockfile) bool +} + +// IsNil checks if lockfile is nil +func IsNil(l Lockfile) bool { + return l == nil || reflect.ValueOf(l).IsNil() } // Package Structure representing a possible Pack @@ -30,3 +40,20 @@ type Package struct { // Set to true iff Key and Version are set Found bool } + +// ByKey sort package structures by key +type ByKey []Package + +func (p ByKey) Len() int { + return len(p) +} + +func (p ByKey) Swap(i, j int) { + p[i], p[j] = p[j], p[i] +} + +func (p ByKey) Less(i, j int) bool { + return p[i].Key < p[j].Key +} + +var _ (sort.Interface) = (*ByKey)(nil) diff --git a/cli/internal/lockfile/npm_lockfile.go b/cli/internal/lockfile/npm_lockfile.go index b53d4278ec5548..4343e97271930d 100644 --- a/cli/internal/lockfile/npm_lockfile.go +++ b/cli/internal/lockfile/npm_lockfile.go @@ -253,6 +253,15 @@ func DecodeNpmLockfile(content []byte) (*NpmLockfile, error) { return &lockfile, nil } +// GlobalChange checks if there are any differences between lockfiles that would completely invalidate +// the cache. +func (l *NpmLockfile) GlobalChange(other Lockfile) bool { + o, ok := other.(*NpmLockfile) + return !ok || + l.LockfileVersion != o.LockfileVersion || + l.Requires != o.Requires +} + // returns a list of possible keys for a dependency of package key func possibleNpmDeps(key string, dep string) []string { possibleDeps := []string{fmt.Sprintf("%s/node_modules/%s", key, dep)} diff --git a/cli/internal/lockfile/pnpm_lockfile.go b/cli/internal/lockfile/pnpm_lockfile.go index 6847d6e1c0b4c6..7a574547392a96 100644 --- a/cli/internal/lockfile/pnpm_lockfile.go +++ b/cli/internal/lockfile/pnpm_lockfile.go @@ -3,6 +3,7 @@ package lockfile import ( "fmt" "io" + "reflect" "sort" "strings" @@ -412,6 +413,17 @@ func (p *PnpmLockfile) Patches() []turbopath.AnchoredUnixPath { return patchPaths } +// GlobalChange checks if there are any differences between lockfiles that would completely invalidate +// the cache. +func (p *PnpmLockfile) GlobalChange(other Lockfile) bool { + o, ok := other.(*PnpmLockfile) + return !ok || + p.Version != o.Version || + p.PackageExtensionsChecksum != o.PackageExtensionsChecksum || + !reflect.DeepEqual(p.Overrides, o.Overrides) || + !reflect.DeepEqual(p.PatchedDependencies, o.PatchedDependencies) +} + func (p *PnpmLockfile) resolveSpecifier(workspacePath turbopath.AnchoredUnixPath, name string, specifier string) (string, bool, error) { pnpmWorkspacePath := workspacePath.ToString() importer, ok := p.Importers[pnpmWorkspacePath] diff --git a/cli/internal/lockfile/pnpm_lockfile_test.go b/cli/internal/lockfile/pnpm_lockfile_test.go index ba30b79d70851a..1741356fab80a4 100644 --- a/cli/internal/lockfile/pnpm_lockfile_test.go +++ b/cli/internal/lockfile/pnpm_lockfile_test.go @@ -7,7 +7,6 @@ import ( "github.com/google/go-cmp/cmp/cmpopts" "github.com/pkg/errors" - "github.com/vercel/turbo/cli/internal/fs" "github.com/vercel/turbo/cli/internal/turbopath" "github.com/vercel/turbo/cli/internal/yaml" "gotest.tools/v3/assert" @@ -18,10 +17,7 @@ func getFixture(t *testing.T, name string) ([]byte, error) { if err != nil { t.Errorf("failed to get cwd: %v", err) } - cwd, err := fs.CheckedToAbsoluteSystemPath(defaultCwd) - if err != nil { - t.Fatalf("cwd is not an absolute directory %v: %v", defaultCwd, err) - } + cwd := turbopath.AbsoluteSystemPath(defaultCwd) lockfilePath := cwd.UntypedJoin("testdata", name) if !lockfilePath.FileExists() { return nil, errors.Errorf("unable to find 'testdata/%s'", name) diff --git a/cli/internal/lockfile/yarn_lockfile.go b/cli/internal/lockfile/yarn_lockfile.go index bbf4104b01d83e..99d776441a5fa5 100644 --- a/cli/internal/lockfile/yarn_lockfile.go +++ b/cli/internal/lockfile/yarn_lockfile.go @@ -106,6 +106,13 @@ func DecodeYarnLockfile(contents []byte) (*YarnLockfile, error) { return &YarnLockfile{lockfile, hasCRLF}, nil } +// GlobalChange checks if there are any differences between lockfiles that would completely invalidate +// the cache. +func (l *YarnLockfile) GlobalChange(other Lockfile) bool { + _, ok := other.(*YarnLockfile) + return !ok +} + func yarnPossibleKeys(name string, version string) []string { return []string{ fmt.Sprintf("%v@%v", name, version), diff --git a/cli/internal/packagemanager/berry.go b/cli/internal/packagemanager/berry.go index 7062ed291496df..17a7f13777ba07 100644 --- a/cli/internal/packagemanager/berry.go +++ b/cli/internal/packagemanager/berry.go @@ -113,7 +113,7 @@ var nodejsBerry = PackageManager{ return true, nil }, - readLockfile: func(contents []byte) (lockfile.Lockfile, error) { + UnmarshalLockfile: func(contents []byte) (lockfile.Lockfile, error) { return lockfile.DecodeBerryLockfile(contents) }, diff --git a/cli/internal/packagemanager/npm.go b/cli/internal/packagemanager/npm.go index 1242365cd7ef71..e4f5f6e1734e91 100644 --- a/cli/internal/packagemanager/npm.go +++ b/cli/internal/packagemanager/npm.go @@ -53,7 +53,7 @@ var nodejsNpm = PackageManager{ return true, nil }, - readLockfile: func(contents []byte) (lockfile.Lockfile, error) { + UnmarshalLockfile: func(contents []byte) (lockfile.Lockfile, error) { return lockfile.DecodeNpmLockfile(contents) }, } diff --git a/cli/internal/packagemanager/packagemanager.go b/cli/internal/packagemanager/packagemanager.go index e536ea5fb16399..8f75263293643a 100644 --- a/cli/internal/packagemanager/packagemanager.go +++ b/cli/internal/packagemanager/packagemanager.go @@ -61,7 +61,7 @@ type PackageManager struct { detect func(projectDirectory turbopath.AbsoluteSystemPath, packageManager *PackageManager) (bool, error) // Read a lockfile for a given package manager - readLockfile func(contents []byte) (lockfile.Lockfile, error) + UnmarshalLockfile func(contents []byte) (lockfile.Lockfile, error) // Prune the given pkgJSON to only include references to the given patches prunePatches func(pkgJSON *fs.PackageJSON, patches []turbopath.AnchoredUnixPath) error @@ -174,15 +174,14 @@ func (pm PackageManager) CanPrune(projectDirectory turbopath.AbsoluteSystemPath) // ReadLockfile will read the applicable lockfile into memory func (pm PackageManager) ReadLockfile(projectDirectory turbopath.AbsoluteSystemPath) (lockfile.Lockfile, error) { - if pm.readLockfile == nil { + if pm.UnmarshalLockfile == nil { return nil, nil } contents, err := projectDirectory.UntypedJoin(pm.Lockfile).ReadFile() if err != nil { return nil, fmt.Errorf("reading %s: %w", pm.Lockfile, err) } - - return pm.readLockfile(contents) + return pm.UnmarshalLockfile(contents) } // PrunePatchedPackages will alter the provided pkgJSON to only reference the provided patches diff --git a/cli/internal/packagemanager/pnpm.go b/cli/internal/packagemanager/pnpm.go index 8eb88094168f52..2c9e5e960b8509 100644 --- a/cli/internal/packagemanager/pnpm.go +++ b/cli/internal/packagemanager/pnpm.go @@ -117,7 +117,7 @@ var nodejsPnpm = PackageManager{ return true, nil }, - readLockfile: func(contents []byte) (lockfile.Lockfile, error) { + UnmarshalLockfile: func(contents []byte) (lockfile.Lockfile, error) { return lockfile.DecodePnpmLockfile(contents) }, diff --git a/cli/internal/packagemanager/pnpm6.go b/cli/internal/packagemanager/pnpm6.go index 924969930908f7..a8f3197c8b3bea 100644 --- a/cli/internal/packagemanager/pnpm6.go +++ b/cli/internal/packagemanager/pnpm6.go @@ -56,7 +56,7 @@ var nodejsPnpm6 = PackageManager{ return true, nil }, - readLockfile: func(contents []byte) (lockfile.Lockfile, error) { + UnmarshalLockfile: func(contents []byte) (lockfile.Lockfile, error) { return lockfile.DecodePnpmLockfile(contents) }, } diff --git a/cli/internal/packagemanager/yarn.go b/cli/internal/packagemanager/yarn.go index d1bef174da16fb..ce3e346ff16448 100644 --- a/cli/internal/packagemanager/yarn.go +++ b/cli/internal/packagemanager/yarn.go @@ -96,7 +96,7 @@ var nodejsYarn = PackageManager{ return packageManager.Matches(packageManager.Slug, strings.TrimSpace(string(out))) }, - readLockfile: func(contents []byte) (lockfile.Lockfile, error) { + UnmarshalLockfile: func(contents []byte) (lockfile.Lockfile, error) { return lockfile.DecodeYarnLockfile(contents) }, } diff --git a/cli/internal/prune/prune.go b/cli/internal/prune/prune.go index e3cb93bbd16e6b..f1db2201d679ad 100644 --- a/cli/internal/prune/prune.go +++ b/cli/internal/prune/prune.go @@ -8,6 +8,7 @@ import ( "github.com/vercel/turbo/cli/internal/cmdutil" "github.com/vercel/turbo/cli/internal/context" "github.com/vercel/turbo/cli/internal/fs" + "github.com/vercel/turbo/cli/internal/lockfile" "github.com/vercel/turbo/cli/internal/turbopath" "github.com/vercel/turbo/cli/internal/turbostate" "github.com/vercel/turbo/cli/internal/ui" @@ -96,7 +97,7 @@ func (p *prune) prune(opts *turbostate.PrunePayload) error { if !canPrune { return errors.Errorf("this command is not yet implemented for %s", ctx.PackageManager.Name) } - if ctx.Lockfile == nil { + if lockfile.IsNil(ctx.Lockfile) { return errors.New("Cannot prune without parsed lockfile") } @@ -128,7 +129,9 @@ func (p *prune) prune(opts *turbostate.PrunePayload) error { p.base.Logger.Trace("targets", "value", targets) lockfileKeys := make([]string, 0, len(rootPackageJSON.TransitiveDeps)) - lockfileKeys = append(lockfileKeys, rootPackageJSON.TransitiveDeps...) + for _, pkg := range rootPackageJSON.TransitiveDeps { + lockfileKeys = append(lockfileKeys, pkg.Key) + } for _, internalDep := range targets { // We skip over the pseudo root node and the root package @@ -160,7 +163,9 @@ func (p *prune) prune(opts *turbostate.PrunePayload) error { } } - lockfileKeys = append(lockfileKeys, ctx.WorkspaceInfos[internalDep].TransitiveDeps...) + for _, pkg := range ctx.WorkspaceInfos[internalDep].TransitiveDeps { + lockfileKeys = append(lockfileKeys, pkg.Key) + } p.base.UI.Output(fmt.Sprintf(" - Added %v", ctx.WorkspaceInfos[internalDep].Name)) } diff --git a/cli/internal/scm/git.go b/cli/internal/scm/git.go index e18a4f4b60a029..6be7e8bcd3641f 100644 --- a/cli/internal/scm/git.go +++ b/cli/internal/scm/git.go @@ -75,6 +75,19 @@ func (g *git) ChangedFiles(fromCommit string, toCommit string, includeUntracked return normalized, nil } +func (g *git) PreviousContent(fromCommit string, filePath string) ([]byte, error) { + if fromCommit == "" { + return nil, fmt.Errorf("Need commit sha to inspect file contents") + } + + out, err := exec.Command("git", "show", fmt.Sprintf("%s:%s", fromCommit, filePath)).CombinedOutput() + if err != nil { + return nil, errors.Wrapf(err, "unable to get contents of %s", filePath) + } + + return out, nil +} + func commitExists(commit string) (bool, error) { err := exec.Command("git", "cat-file", "-t", commit).Run() if err != nil { diff --git a/cli/internal/scm/scm.go b/cli/internal/scm/scm.go index 70ecd5a3ee4f4d..4790633a4898f0 100644 --- a/cli/internal/scm/scm.go +++ b/cli/internal/scm/scm.go @@ -21,6 +21,8 @@ var ErrFallback = errors.New("cannot find a .git folder. Falling back to manual type SCM interface { // ChangedFiles returns a list of modified files since the given commit, optionally including untracked files.*/ ChangedFiles(fromCommit string, toCommit string, includeUntracked bool, relativeTo string) ([]string, error) + // PreviousContent Returns the content of the file at fromCommit + PreviousContent(fromCommit string, filePath string) ([]byte, error) } // newGitSCM returns a new SCM instance for this repo root. diff --git a/cli/internal/scm/stub.go b/cli/internal/scm/stub.go index 5b5994802a8a74..6b84b382dcd93b 100644 --- a/cli/internal/scm/stub.go +++ b/cli/internal/scm/stub.go @@ -8,3 +8,7 @@ type stub struct{} func (s *stub) ChangedFiles(fromCommit string, toCommit string, includeUntracked bool, relativeTo string) ([]string, error) { return nil, nil } + +func (s *stub) PreviousContent(fromCommit string, filePath string) ([]byte, error) { + return nil, nil +} diff --git a/cli/internal/scope/scope.go b/cli/internal/scope/scope.go index d92c8ac347c103..204ef7ed08ae4b 100644 --- a/cli/internal/scope/scope.go +++ b/cli/internal/scope/scope.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "path/filepath" + "sort" "strings" "github.com/hashicorp/go-hclog" @@ -11,7 +12,7 @@ import ( "github.com/pkg/errors" "github.com/vercel/turbo/cli/internal/context" "github.com/vercel/turbo/cli/internal/graph" - "github.com/vercel/turbo/cli/internal/packagemanager" + "github.com/vercel/turbo/cli/internal/lockfile" "github.com/vercel/turbo/cli/internal/scm" scope_filter "github.com/vercel/turbo/cli/internal/scope/filter" "github.com/vercel/turbo/cli/internal/turbopath" @@ -127,7 +128,7 @@ func ResolvePackages(opts *Opts, repoRoot turbopath.AbsoluteSystemPath, scm scm. WorkspaceInfos: ctx.WorkspaceInfos, Cwd: repoRoot, Inference: inferenceBase, - PackagesChangedInRange: opts.getPackageChangeFunc(scm, repoRoot, ctx.WorkspaceInfos, ctx.PackageManager), + PackagesChangedInRange: opts.getPackageChangeFunc(scm, repoRoot, ctx), } filterPatterns := opts.FilterPatterns legacyFilterPatterns := opts.LegacyFilter.asFilterPatterns() @@ -190,7 +191,7 @@ func calculateInference(repoRoot turbopath.AbsoluteSystemPath, rawPkgInferenceDi }, nil } -func (o *Opts) getPackageChangeFunc(scm scm.SCM, cwd turbopath.AbsoluteSystemPath, packageInfos graph.WorkspaceInfos, packageManager *packagemanager.PackageManager) scope_filter.PackagesChangedInRange { +func (o *Opts) getPackageChangeFunc(scm scm.SCM, cwd turbopath.AbsoluteSystemPath, ctx *context.Context) scope_filter.PackagesChangedInRange { return func(fromRef string, toRef string) (util.Set, error) { // We could filter changed files at the git level, since it's possible // that the changes we're interested in are scoped, but we need to handle @@ -202,37 +203,85 @@ func (o *Opts) getPackageChangeFunc(scm scm.SCM, cwd turbopath.AbsoluteSystemPat if err != nil { return nil, err } + sort.Strings(scmChangedFiles) changedFiles = scmChangedFiles } - if hasRepoGlobalFileChanged, err := repoGlobalFileHasChanged(o, getDefaultGlobalDeps(packageManager), changedFiles); err != nil { - return nil, err - } else if hasRepoGlobalFileChanged { + makeAllPkgs := func() util.Set { allPkgs := make(util.Set) - for pkg := range packageInfos { + for pkg := range ctx.WorkspaceInfos { allPkgs.Add(pkg) } - return allPkgs, nil + return allPkgs + } + if hasRepoGlobalFileChanged, err := repoGlobalFileHasChanged(o, getDefaultGlobalDeps(), changedFiles); err != nil { + return nil, err + } else if hasRepoGlobalFileChanged { + return makeAllPkgs(), nil } + filteredChangedFiles, err := filterIgnoredFiles(o, changedFiles) if err != nil { return nil, err } - changedPkgs := getChangedPackages(filteredChangedFiles, packageInfos) + changedPkgs := getChangedPackages(filteredChangedFiles, ctx.WorkspaceInfos) + + if lockfileChanges, fullChanges := getChangesFromLockfile(scm, ctx, changedFiles, fromRef); !fullChanges { + for _, pkg := range lockfileChanges { + changedPkgs.Add(pkg) + } + } else { + return makeAllPkgs(), nil + } + return changedPkgs, nil } } -func getDefaultGlobalDeps(packageManager *packagemanager.PackageManager) []string { - // include turbo.json, root package.json, and root lockfile as implicit global dependencies +func getChangesFromLockfile(scm scm.SCM, ctx *context.Context, changedFiles []string, fromRef string) ([]string, bool) { + lockfileFilter, err := filter.Compile([]string{ctx.PackageManager.Lockfile}) + if err != nil { + panic(fmt.Sprintf("Lockfile is invalid glob: %v", err)) + } + match := false + for _, file := range changedFiles { + if lockfileFilter.Match(file) { + match = true + break + } + } + if !match { + return nil, false + } + + if lockfile.IsNil(ctx.Lockfile) { + return nil, true + } + + prevContents, err := scm.PreviousContent(fromRef, ctx.PackageManager.Lockfile) + if err != nil { + // unable to reconstruct old lockfile, assume everything changed + return nil, true + } + prevLockfile, err := ctx.PackageManager.UnmarshalLockfile(prevContents) + if err != nil { + // unable to parse old lockfile, assume everything changed + return nil, true + } + additionalPkgs, err := ctx.ChangedPackages(prevLockfile) + if err != nil { + // missing at least one lockfile, assume everything changed + return nil, true + } + + return additionalPkgs, false +} + +func getDefaultGlobalDeps() []string { + // include turbo.json and root package.json as implicit global dependencies defaultGlobalDeps := []string{ "turbo.json", "package.json", } - if packageManager != nil { - // TODO: we should be smarter here and determine if the lockfile changes actually impact the given scope - defaultGlobalDeps = append(defaultGlobalDeps, packageManager.Lockfile) - } - return defaultGlobalDeps } diff --git a/cli/internal/scope/scope_test.go b/cli/internal/scope/scope_test.go index ce6a20df334b7f..c904907c085bfa 100644 --- a/cli/internal/scope/scope_test.go +++ b/cli/internal/scope/scope_test.go @@ -2,6 +2,7 @@ package scope import ( "fmt" + "io" "os" "path/filepath" "reflect" @@ -12,6 +13,7 @@ import ( "github.com/vercel/turbo/cli/internal/context" "github.com/vercel/turbo/cli/internal/fs" internalGraph "github.com/vercel/turbo/cli/internal/graph" + "github.com/vercel/turbo/cli/internal/lockfile" "github.com/vercel/turbo/cli/internal/packagemanager" "github.com/vercel/turbo/cli/internal/turbopath" "github.com/vercel/turbo/cli/internal/ui" @@ -19,13 +21,60 @@ import ( ) type mockSCM struct { - changed []string + changed []string + contents map[string][]byte } func (m *mockSCM) ChangedFiles(_fromCommit string, _toCommit string, _includeUntracked bool, _relativeTo string) ([]string, error) { return m.changed, nil } +func (m *mockSCM) PreviousContent(fromCommit string, filePath string) ([]byte, error) { + contents, ok := m.contents[filePath] + if !ok { + return nil, fmt.Errorf("No contents found") + } + return contents, nil +} + +type mockLockfile struct { + globalChange bool + versions map[string]string + allDeps map[string]map[string]string +} + +func (m *mockLockfile) ResolvePackage(workspacePath turbopath.AnchoredUnixPath, name string, version string) (lockfile.Package, error) { + resolvedVersion, ok := m.versions[name] + if ok { + key := fmt.Sprintf("%s%s", name, version) + return lockfile.Package{Key: key, Version: resolvedVersion, Found: true}, nil + } + return lockfile.Package{Found: false}, nil +} + +func (m *mockLockfile) AllDependencies(key string) (map[string]string, bool) { + deps, ok := m.allDeps[key] + return deps, ok +} + +func (m *mockLockfile) Encode(w io.Writer) error { + return nil +} + +func (m *mockLockfile) GlobalChange(other lockfile.Lockfile) bool { + return m.globalChange || (other != nil && other.(*mockLockfile).globalChange) +} + +func (m *mockLockfile) Patches() []turbopath.AnchoredUnixPath { + return nil +} + +func (m *mockLockfile) Subgraph(workspaces []turbopath.AnchoredSystemPath, packages []string) (lockfile.Lockfile, error) { + return nil, nil +} + +var _ (lockfile.Lockfile) = (*mockLockfile)(nil) + func TestResolvePackages(t *testing.T) { cwd, err := os.Getwd() if err != nil { @@ -81,12 +130,18 @@ func TestResolvePackages(t *testing.T) { graph.Connect(dag.BasicEdge("app2-a", "libC")) workspaceInfos := internalGraph.WorkspaceInfos{ "//": { - Dir: turbopath.AnchoredSystemPath("").ToSystemPath(), - Name: "monorepo", + Dir: turbopath.AnchoredSystemPath("").ToSystemPath(), + UnresolvedExternalDeps: map[string]string{"global": "2"}, + TransitiveDeps: []lockfile.Package{{Key: "global2", Version: "2", Found: true}}, }, "app0": { - Dir: turbopath.AnchoredUnixPath("app/app0").ToSystemPath(), - Name: "app0", + Dir: turbopath.AnchoredUnixPath("app/app0").ToSystemPath(), + Name: "app0", + UnresolvedExternalDeps: map[string]string{"app0-dep": "2"}, + TransitiveDeps: []lockfile.Package{ + {Key: "app0-dep2", Version: "2", Found: true}, + {Key: "app0-util2", Version: "2", Found: true}, + }, }, "app1": { Dir: turbopath.AnchoredUnixPath("app/app1").ToSystemPath(), @@ -105,8 +160,14 @@ func TestResolvePackages(t *testing.T) { Name: "libA", }, "libB": { - Dir: turbopath.AnchoredUnixPath("libs/libB").ToSystemPath(), - Name: "libB", + Dir: turbopath.AnchoredUnixPath("libs/libB").ToSystemPath(), + Name: "libB", + UnresolvedExternalDeps: map[string]string{"external": "1"}, + TransitiveDeps: []lockfile.Package{ + {Key: "external-dep-a1", Version: "1", Found: true}, + {Key: "external-dep-b1", Version: "1", Found: true}, + {Key: "external1", Version: "1", Found: true}, + }, }, "libC": { Dir: turbopath.AnchoredUnixPath("libs/libC").ToSystemPath(), @@ -122,6 +183,40 @@ func TestResolvePackages(t *testing.T) { packageNames = append(packageNames, name) } + // global -> globalDep + // app0-dep -> app0-dep :) + + makeLockfile := func(f func(*mockLockfile)) *mockLockfile { + l := mockLockfile{ + globalChange: false, + versions: map[string]string{ + "global": "2", + "app0-dep": "2", + "app0-util": "2", + "external": "1", + "external-dep-a": "1", + "external-dep-b": "1", + }, + allDeps: map[string]map[string]string{ + "global2": map[string]string{}, + "app0-dep2": map[string]string{ + "app0-util": "2", + }, + "app0-util2": map[string]string{}, + "external1": map[string]string{ + "external-dep-a": "1", + "external-dep-b": "1", + }, + "external-dep-a1": map[string]string{}, + "external-dep-b1": map[string]string{}, + }, + } + if f != nil { + f(&l) + } + return &l + } + testCases := []struct { name string changed []string @@ -134,6 +229,8 @@ func TestResolvePackages(t *testing.T) { includeDependencies bool includeDependents bool lockfile string + currLockfile *mockLockfile + prevLockfile *mockLockfile inferPkgPath string }{ { @@ -187,6 +284,12 @@ func TestResolvePackages(t *testing.T) { expected: []string{"libB"}, since: "dummy", }, + { + name: "One package manifest changed", + changed: []string{"libs/libB/package.json"}, + expected: []string{"libB"}, + since: "dummy", + }, { name: "An ignored package changed", changed: []string{"libs/libB/src/index.ts"}, @@ -290,6 +393,69 @@ func TestResolvePackages(t *testing.T) { expected: []string{"app2", "app2-a"}, since: "dummy", }, + { + name: "Global lockfile change invalidates all packages", + changed: []string{"dummy.lock"}, + expected: []string{"//", "app0", "app1", "app2", "app2-a", "libA", "libB", "libC", "libD"}, + lockfile: "dummy.lock", + currLockfile: makeLockfile(nil), + prevLockfile: makeLockfile(func(ml *mockLockfile) { + ml.globalChange = true + }), + since: "dummy", + }, + { + name: "Dependency of workspace root change invalidates all packages", + changed: []string{"dummy.lock"}, + expected: []string{"//", "app0", "app1", "app2", "app2-a", "libA", "libB", "libC", "libD"}, + lockfile: "dummy.lock", + currLockfile: makeLockfile(nil), + prevLockfile: makeLockfile(func(ml *mockLockfile) { + ml.versions["global"] = "3" + ml.allDeps["global3"] = map[string]string{} + }), + since: "dummy", + }, + { + name: "Version change invalidates package", + changed: []string{"dummy.lock"}, + expected: []string{"//", "app0"}, + lockfile: "dummy.lock", + currLockfile: makeLockfile(nil), + prevLockfile: makeLockfile(func(ml *mockLockfile) { + ml.versions["app0-util"] = "3" + ml.allDeps["app0-dep2"] = map[string]string{"app0-util": "3"} + ml.allDeps["app0-util3"] = map[string]string{} + }), + since: "dummy", + }, + { + name: "Transitive dep invalidates package", + changed: []string{"dummy.lock"}, + expected: []string{"//", "libB"}, + lockfile: "dummy.lock", + currLockfile: makeLockfile(nil), + prevLockfile: makeLockfile(func(ml *mockLockfile) { + ml.versions["external-dep-a"] = "2" + ml.allDeps["external1"] = map[string]string{"external-dep-a": "2", "external-dep-b": "1"} + ml.allDeps["external-dep-a2"] = map[string]string{} + }), + since: "dummy", + }, + { + name: "Transitive dep invalidates package and dependents", + changed: []string{"dummy.lock"}, + expected: []string{"//", "app0", "app1", "app2", "libA", "libB"}, + lockfile: "dummy.lock", + includeDependents: true, + currLockfile: makeLockfile(nil), + prevLockfile: makeLockfile(func(ml *mockLockfile) { + ml.versions["external-dep-a"] = "2" + ml.allDeps["external1"] = map[string]string{"external-dep-a": "2", "external-dep-b": "1"} + ml.allDeps["external-dep-a2"] = map[string]string{} + }), + since: "dummy", + }, { name: "Infer app2 from directory", inferPkgPath: "app/app2", @@ -333,7 +499,14 @@ func TestResolvePackages(t *testing.T) { systemSeparatorChanged[index] = filepath.FromSlash(path) } scm := &mockSCM{ - changed: systemSeparatorChanged, + changed: systemSeparatorChanged, + contents: make(map[string][]byte, len(systemSeparatorChanged)), + } + for _, path := range systemSeparatorChanged { + scm.contents[path] = nil + } + readLockfile := func(contents []byte) (lockfile.Lockfile, error) { + return tc.prevLockfile, nil } pkgs, isAllPackages, err := ResolvePackages(&Opts{ LegacyFilter: LegacyFilter{ @@ -348,8 +521,10 @@ func TestResolvePackages(t *testing.T) { }, root, scm, &context.Context{ WorkspaceInfos: workspaceInfos, WorkspaceNames: packageNames, - PackageManager: &packagemanager.PackageManager{Lockfile: tc.lockfile}, + PackageManager: &packagemanager.PackageManager{Lockfile: tc.lockfile, UnmarshalLockfile: readLockfile}, WorkspaceGraph: graph, + RootNode: "root", + Lockfile: tc.currLockfile, }, tui, logger) if err != nil { t.Errorf("expected no error, got %v", err)