From 2f7d3dd6fb45520b1ab7e474e4020fc5d69bed78 Mon Sep 17 00:00:00 2001 From: Colin Cross Date: Wed, 28 Apr 2021 11:29:33 -0700 Subject: [PATCH] Make AddVariationDependencies return list of nil for non-parallel mutators According to the documentation, Add[Variation]Dependencies should be returning a list the same length as the input list. A non-parallel mutator doesn't support pausing to wait for new dependencies to finish the current mutator pass, so it can't return the new dependencies. Make it return a list of nil instead to match the contract. Bug: 186539038 Test: TestAddVariationDependencies Change-Id: I07d0ac1f3024dcb7c94a2518910a68b61cd731e2 --- module_ctx.go | 18 ++++-- module_ctx_test.go | 144 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 156 insertions(+), 6 deletions(-) diff --git a/module_ctx.go b/module_ctx.go index 8b31c10..da07062 100644 --- a/module_ctx.go +++ b/module_ctx.go @@ -1102,9 +1102,11 @@ func (mctx *mutatorContext) AddDependency(module Module, tag DependencyTag, deps if len(errs) > 0 { mctx.errs = append(mctx.errs, errs...) } - if mctx.pause(depInfo) { - depInfos = append(depInfos, maybeLogicModule(depInfo)) + if !mctx.pause(depInfo) { + // Pausing not supported by this mutator, new dependencies can't be returned. + depInfo = nil } + depInfos = append(depInfos, maybeLogicModule(depInfo)) } return depInfos } @@ -1135,9 +1137,11 @@ func (mctx *mutatorContext) AddVariationDependencies(variations []Variation, tag if len(errs) > 0 { mctx.errs = append(mctx.errs, errs...) } - if mctx.pause(depInfo) { - depInfos = append(depInfos, maybeLogicModule(depInfo)) + if !mctx.pause(depInfo) { + // Pausing not supported by this mutator, new dependencies can't be returned. + depInfo = nil } + depInfos = append(depInfos, maybeLogicModule(depInfo)) } return depInfos } @@ -1151,9 +1155,11 @@ func (mctx *mutatorContext) AddFarVariationDependencies(variations []Variation, if len(errs) > 0 { mctx.errs = append(mctx.errs, errs...) } - if mctx.pause(depInfo) { - depInfos = append(depInfos, maybeLogicModule(depInfo)) + if !mctx.pause(depInfo) { + // Pausing not supported by this mutator, new dependencies can't be returned. + depInfo = nil } + depInfos = append(depInfos, maybeLogicModule(depInfo)) } return depInfos } diff --git a/module_ctx_test.go b/module_ctx_test.go index dafface..d57982e 100644 --- a/module_ctx_test.go +++ b/module_ctx_test.go @@ -68,6 +68,15 @@ func addVariantDepsMutator(variants []Variation, tag DependencyTag, from, to str } } +func addVariantDepsResultMutator(variants []Variation, tag DependencyTag, from, to string, results map[string][]Module) func(ctx BottomUpMutatorContext) { + return func(ctx BottomUpMutatorContext) { + if ctx.ModuleName() == from { + ret := ctx.AddVariationDependencies(variants, tag, to) + results[ctx.ModuleName()] = ret + } + } +} + func TestAliasVariation(t *testing.T) { runWithFailures := func(ctx *Context, expectedErr string) { t.Helper() @@ -316,6 +325,141 @@ func expectedErrors(t *testing.T, errs []error, expectedMessages ...string) { } } +func TestAddVariationDependencies(t *testing.T) { + runWithFailures := func(ctx *Context, expectedErr string) { + t.Helper() + bp := ` + test { + name: "foo", + } + + test { + name: "bar", + } + ` + + mockFS := map[string][]byte{ + "Blueprints": []byte(bp), + } + + ctx.MockFileSystem(mockFS) + + _, errs := ctx.ParseFileList(".", []string{"Blueprints"}, nil) + if len(errs) > 0 { + t.Errorf("unexpected parse errors:") + for _, err := range errs { + t.Errorf(" %s", err) + } + } + + _, errs = ctx.ResolveDependencies(nil) + if len(errs) > 0 { + if expectedErr == "" { + t.Errorf("unexpected dep errors:") + for _, err := range errs { + t.Errorf(" %s", err) + } + } else { + for _, err := range errs { + if strings.Contains(err.Error(), expectedErr) { + continue + } else { + t.Errorf("unexpected dep error: %s", err) + } + } + } + } else if expectedErr != "" { + t.Errorf("missing dep error: %s", expectedErr) + } + } + + run := func(ctx *Context) { + t.Helper() + runWithFailures(ctx, "") + } + + t.Run("parallel", func(t *testing.T) { + ctx := NewContext() + ctx.RegisterModuleType("test", newModuleCtxTestModule) + results := make(map[string][]Module) + depsMutator := addVariantDepsResultMutator(nil, nil, "foo", "bar", results) + ctx.RegisterBottomUpMutator("deps", depsMutator).Parallel() + + run(ctx) + + foo := ctx.moduleGroupFromName("foo", nil).moduleByVariantName("") + bar := ctx.moduleGroupFromName("bar", nil).moduleByVariantName("") + + if g, w := foo.forwardDeps, []*moduleInfo{bar}; !reflect.DeepEqual(g, w) { + t.Fatalf("expected foo deps to be %q, got %q", w, g) + } + + if g, w := results["foo"], []Module{bar.logicModule}; !reflect.DeepEqual(g, w) { + t.Fatalf("expected AddVariationDependencies return value to be %q, got %q", w, g) + } + }) + + t.Run("non-parallel", func(t *testing.T) { + ctx := NewContext() + ctx.RegisterModuleType("test", newModuleCtxTestModule) + results := make(map[string][]Module) + depsMutator := addVariantDepsResultMutator(nil, nil, "foo", "bar", results) + ctx.RegisterBottomUpMutator("deps", depsMutator) + run(ctx) + + foo := ctx.moduleGroupFromName("foo", nil).moduleByVariantName("") + bar := ctx.moduleGroupFromName("bar", nil).moduleByVariantName("") + + if g, w := foo.forwardDeps, []*moduleInfo{bar}; !reflect.DeepEqual(g, w) { + t.Fatalf("expected foo deps to be %q, got %q", w, g) + } + + if g, w := results["foo"], []Module{nil}; !reflect.DeepEqual(g, w) { + t.Fatalf("expected AddVariationDependencies return value to be %q, got %q", w, g) + } + }) + + t.Run("missing", func(t *testing.T) { + ctx := NewContext() + ctx.RegisterModuleType("test", newModuleCtxTestModule) + results := make(map[string][]Module) + depsMutator := addVariantDepsResultMutator(nil, nil, "foo", "baz", results) + ctx.RegisterBottomUpMutator("deps", depsMutator).Parallel() + runWithFailures(ctx, `"foo" depends on undefined module "baz"`) + + foo := ctx.moduleGroupFromName("foo", nil).moduleByVariantName("") + + if g, w := foo.forwardDeps, []*moduleInfo(nil); !reflect.DeepEqual(g, w) { + t.Fatalf("expected foo deps to be %q, got %q", w, g) + } + + if g, w := results["foo"], []Module{nil}; !reflect.DeepEqual(g, w) { + t.Fatalf("expected AddVariationDependencies return value to be %q, got %q", w, g) + } + }) + + t.Run("allow missing", func(t *testing.T) { + ctx := NewContext() + ctx.SetAllowMissingDependencies(true) + ctx.RegisterModuleType("test", newModuleCtxTestModule) + results := make(map[string][]Module) + depsMutator := addVariantDepsResultMutator(nil, nil, "foo", "baz", results) + ctx.RegisterBottomUpMutator("deps", depsMutator).Parallel() + run(ctx) + + foo := ctx.moduleGroupFromName("foo", nil).moduleByVariantName("") + + if g, w := foo.forwardDeps, []*moduleInfo(nil); !reflect.DeepEqual(g, w) { + t.Fatalf("expected foo deps to be %q, got %q", w, g) + } + + if g, w := results["foo"], []Module{nil}; !reflect.DeepEqual(g, w) { + t.Fatalf("expected AddVariationDependencies return value to be %q, got %q", w, g) + } + }) + +} + func TestCheckBlueprintSyntax(t *testing.T) { factories := map[string]ModuleFactory{ "test": newModuleCtxTestModule,