From a7b399cb4c62db44157e3207ea2a77b96e9a2b9f Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 10 Jan 2019 12:20:03 -0500 Subject: [PATCH 1/3] use actual schema.Resources for state shims Provider tests often rely on checking values contained within sets, by directly accessing their flatmapped representation. In order to provider the test harness with the expected set hashes, the sets must be generated by the schema.Resource itself. During the test we now build a fixed map of the providers, which should only contain schema.Provider instances, and pass them into each TestStep. The individual schema.Resource instances can then be pulled from the providers, and used to recreate the state from the cty.Value returned by the core operations. --- helper/resource/state_shim.go | 82 +++++++++++++------------ helper/resource/state_shim_test.go | 49 +++++---------- helper/resource/testing.go | 66 ++++++++++---------- helper/resource/testing_config.go | 13 ++-- helper/resource/testing_import_state.go | 6 +- 5 files changed, 100 insertions(+), 116 deletions(-) diff --git a/helper/resource/state_shim.go b/helper/resource/state_shim.go index 13802dbc7..264928d83 100644 --- a/helper/resource/state_shim.go +++ b/helper/resource/state_shim.go @@ -4,25 +4,17 @@ import ( "fmt" "github.com/hashicorp/terraform/addrs" - "github.com/hashicorp/terraform/configs/configschema" "github.com/zclconf/go-cty/cty" "github.com/hashicorp/terraform/config/hcl2shim" + "github.com/hashicorp/terraform/helper/schema" "github.com/hashicorp/terraform/states" "github.com/hashicorp/terraform/terraform" ) -func mustShimNewState(newState *states.State, schemas *terraform.Schemas) *terraform.State { - s, err := shimNewState(newState, schemas) - if err != nil { - panic(err) - } - return s -} - // shimState takes a new *states.State and reverts it to a legacy state for the provider ACC tests -func shimNewState(newState *states.State, schemas *terraform.Schemas) (*terraform.State, error) { +func shimNewState(newState *states.State, providers map[string]terraform.ResourceProvider) (*terraform.State, error) { state := terraform.NewState() // in the odd case of a nil state, let the helper packages handle it @@ -57,25 +49,10 @@ func shimNewState(newState *states.State, schemas *terraform.Schemas) (*terrafor resType := res.Addr.Type providerType := res.ProviderConfig.ProviderConfig.Type - providerSchema := schemas.Providers[providerType] - if providerSchema == nil { - return nil, fmt.Errorf("missing schema for %q", providerType) - } - - var resSchema *configschema.Block - switch res.Addr.Mode { - case addrs.ManagedResourceMode: - resSchema = providerSchema.ResourceTypes[resType] - case addrs.DataResourceMode: - resSchema = providerSchema.DataSources[resType] - } - - if resSchema == nil { - return nil, fmt.Errorf("missing resource schema for %q in %q", resType, providerType) - } + resource := getResource(providers, providerType, resType) for key, i := range res.Instances { - flatmap, err := shimmedAttributes(i.Current, resSchema.ImpliedType()) + flatmap, err := shimmedAttributes(i.Current, resource) if err != nil { return nil, fmt.Errorf("error decoding state for %q: %s", resType, err) } @@ -114,7 +91,7 @@ func shimNewState(newState *states.State, schemas *terraform.Schemas) (*terrafor // add any deposed instances for _, dep := range i.Deposed { - flatmap, err := shimmedAttributes(dep, resSchema.ImpliedType()) + flatmap, err := shimmedAttributes(dep, resource) if err != nil { return nil, fmt.Errorf("error decoding deposed state for %q: %s", resType, err) } @@ -139,17 +116,46 @@ func shimNewState(newState *states.State, schemas *terraform.Schemas) (*terrafor return state, nil } -func shimmedAttributes(instance *states.ResourceInstanceObjectSrc, ty cty.Type) (map[string]string, error) { +func getResource(providers map[string]terraform.ResourceProvider, providerName, resourceType string) *schema.Resource { + p := providers[providerName] + if p == nil { + panic(fmt.Sprintf("provider %q not found in test step", providerName)) + } + + // this is only for tests, so should only see schema.Providers + provider := p.(*schema.Provider) + + resource := provider.ResourcesMap[resourceType] + if resource != nil { + return resource + + } + + resource = provider.DataSourcesMap[resourceType] + if resource != nil { + return resource + } + + panic(fmt.Sprintf("resource %s not found in test step", resourceType)) +} + +func shimmedAttributes(instance *states.ResourceInstanceObjectSrc, res *schema.Resource) (map[string]string, error) { flatmap := instance.AttrsFlat - // if we have json attrs, they need to be decoded - if flatmap == nil { - rio, err := instance.Decode(ty) - if err != nil { - return nil, err - } - - flatmap = hcl2shim.FlatmapValueFromHCL2(rio.Value) + if flatmap != nil { + return flatmap, nil } - return flatmap, nil + + // if we have json attrs, they need to be decoded + rio, err := instance.Decode(res.CoreConfigSchema().ImpliedType()) + if err != nil { + return nil, err + } + + instanceState, err := res.ShimInstanceStateFromValue(rio.Value) + if err != nil { + return nil, err + } + + return instanceState.Attributes, nil } diff --git a/helper/resource/state_shim_test.go b/helper/resource/state_shim_test.go index 22f5b02fe..0a0147c76 100644 --- a/helper/resource/state_shim_test.go +++ b/helper/resource/state_shim_test.go @@ -4,7 +4,7 @@ import ( "testing" "github.com/hashicorp/terraform/addrs" - "github.com/hashicorp/terraform/configs/configschema" + "github.com/hashicorp/terraform/helper/schema" "github.com/hashicorp/terraform/states" "github.com/hashicorp/terraform/terraform" "github.com/zclconf/go-cty/cty" @@ -286,46 +286,29 @@ func TestStateShim(t *testing.T) { }, } - schemas := &terraform.Schemas{ - Providers: map[string]*terraform.ProviderSchema{ - "test": { - ResourceTypes: map[string]*configschema.Block{ - "test_thing": &configschema.Block{ - Attributes: map[string]*configschema.Attribute{ - "id": { - Type: cty.String, - Computed: true, - }, - "fizzle": { - Type: cty.String, - Optional: true, - }, - "bazzle": { - Type: cty.String, - Optional: true, - }, - }, + providers := map[string]terraform.ResourceProvider{ + "test": &schema.Provider{ + ResourcesMap: map[string]*schema.Resource{ + "test_thing": &schema.Resource{ + Schema: map[string]*schema.Schema{ + "id": {Type: schema.TypeString, Computed: true}, + "fizzle": {Type: schema.TypeString, Optional: true}, + "bazzle": {Type: schema.TypeString, Optional: true}, }, }, - DataSources: map[string]*configschema.Block{ - "test_data_thing": &configschema.Block{ - Attributes: map[string]*configschema.Attribute{ - "id": { - Type: cty.String, - Computed: true, - }, - "fuzzle": { - Type: cty.String, - Optional: true, - }, - }, + }, + DataSourcesMap: map[string]*schema.Resource{ + "test_data_thing": &schema.Resource{ + Schema: map[string]*schema.Schema{ + "id": {Type: schema.TypeString, Computed: true}, + "fuzzle": {Type: schema.TypeString, Optional: true}, }, }, }, }, } - shimmed, err := shimNewState(state, schemas) + shimmed, err := shimNewState(state, providers) if err != nil { t.Fatal(err) } diff --git a/helper/resource/testing.go b/helper/resource/testing.go index 83ef7713c..fe3df8ddf 100644 --- a/helper/resource/testing.go +++ b/helper/resource/testing.go @@ -382,6 +382,10 @@ type TestStep struct { // be refreshed and don't matter. ImportStateVerify bool ImportStateVerifyIgnore []string + + // provider s is used internally to maintain a reference to the + // underlying providers during the tests + providers map[string]terraform.ResourceProvider } // Set to a file mask in sprintf format where %s is test name @@ -476,6 +480,17 @@ func Test(t TestT, c TestCase) { c.PreCheck() } + // get instances of all providers, so we can use the individual + // resources to shim the state during the tests. + providers := make(map[string]terraform.ResourceProvider) + for name, pf := range testProviderFactories(c) { + p, err := pf() + if err != nil { + t.Fatal(err) + } + providers[name] = p + } + providerResolver, err := testProviderResolver(c) if err != nil { t.Fatal(err) @@ -491,6 +506,10 @@ func Test(t TestT, c TestCase) { idRefresh := c.IDRefreshName != "" errored := false for i, step := range c.Steps { + // insert the providers into the step so we can get the resources for + // shimming the state + step.providers = providers + var err error log.Printf("[DEBUG] Test: Executing step %d", i) @@ -600,6 +619,7 @@ func Test(t TestT, c TestCase) { Destroy: true, PreventDiskCleanup: lastStep.PreventDiskCleanup, PreventPostDestroyRefresh: c.PreventPostDestroyRefresh, + providers: providers, } log.Printf("[WARN] Test: Executing destroy step") @@ -629,12 +649,10 @@ func testProviderConfig(c TestCase) string { return strings.Join(lines, "") } -// testProviderResolver is a helper to build a ResourceProviderResolver -// with pre instantiated ResourceProviders, so that we can reset them for the -// test, while only calling the factory function once. -// Any errors are stored so that they can be returned by the factory in -// terraform to match non-test behavior. -func testProviderResolver(c TestCase) (providers.Resolver, error) { +// testProviderFactories combines the fixed Providers and +// ResourceProviderFactory functions into a single map of +// ResourceProviderFactory functions. +func testProviderFactories(c TestCase) map[string]terraform.ResourceProviderFactory { ctxProviders := make(map[string]terraform.ResourceProviderFactory) for k, pf := range c.ProviderFactories { ctxProviders[k] = pf @@ -644,6 +662,16 @@ func testProviderResolver(c TestCase) (providers.Resolver, error) { for k, p := range c.Providers { ctxProviders[k] = terraform.ResourceProviderFactoryFixed(p) } + return ctxProviders +} + +// testProviderResolver is a helper to build a ResourceProviderResolver +// with pre instantiated ResourceProviders, so that we can reset them for the +// test, while only calling the factory function once. +// Any errors are stored so that they can be returned by the factory in +// terraform to match non-test behavior. +func testProviderResolver(c TestCase) (providers.Resolver, error) { + ctxProviders := testProviderFactories(c) // wrap the old provider factories in the test grpc server so they can be // called from terraform. @@ -667,32 +695,6 @@ func testProviderResolver(c TestCase) (providers.Resolver, error) { return providers.ResolverFixed(newProviders), nil } -// testProviderFactores returns a fixed and reset factories for creating a resolver -func testProviderFactories(c TestCase) (map[string]providers.Factory, error) { - factories := c.ProviderFactories - if factories == nil { - factories = make(map[string]terraform.ResourceProviderFactory) - } - - // add any fixed providers - for k, p := range c.Providers { - factories[k] = terraform.ResourceProviderFactoryFixed(p) - } - - // wrap the providers to be GRPC mocks rather than legacy terraform.ResourceProvider - newFactories := make(map[string]providers.Factory) - for k, pf := range factories { - newFactories[k] = func() (providers.Interface, error) { - p, err := pf() - if err != nil { - return nil, err - } - return GRPCTestProvider(p), nil - } - } - return newFactories, nil -} - // UnitTest is a helper to force the acceptance testing harness to run in the // normal unit test suite. This should only be used for resource that don't // have any external dependencies. diff --git a/helper/resource/testing_config.go b/helper/resource/testing_config.go index fd5324350..7635a7057 100644 --- a/helper/resource/testing_config.go +++ b/helper/resource/testing_config.go @@ -62,14 +62,11 @@ func testStep(opts terraform.ContextOpts, state *terraform.State, step TestStep) log.Printf("[WARN] Config warnings:\n%s", stepDiags) } - // We will need access to the schemas in order to shim to the old-style - // testing API. - schemas := ctx.Schemas() - // Refresh! newState, stepDiags := ctx.Refresh() // shim the state first so the test can check the state on errors - state, err = shimNewState(newState, schemas) + + state, err = shimNewState(newState, step.providers) if err != nil { return nil, err } @@ -95,7 +92,7 @@ func testStep(opts terraform.ContextOpts, state *terraform.State, step TestStep) // Apply the diff, creating real resources. newState, stepDiags = ctx.Apply() // shim the state first so the test can check the state on errors - state, err = shimNewState(newState, schemas) + state, err = shimNewState(newState, step.providers) if err != nil { return nil, err } @@ -139,7 +136,7 @@ func testStep(opts terraform.ContextOpts, state *terraform.State, step TestStep) return state, newOperationError("follow-up refresh", stepDiags) } - state, err = shimNewState(newState, schemas) + state, err = shimNewState(newState, step.providers) if err != nil { return nil, err } @@ -190,7 +187,7 @@ func testStep(opts terraform.ContextOpts, state *terraform.State, step TestStep) // // This is here only for compatibility with existing tests that predate our // new plan and state types, and should not be used in new tests. Instead, use -// a library like "cmp" to do a deep equality check and diff on the two +// a library like "cmp" to do a deep equality and diff on the two // data structures. func legacyPlanComparisonString(state *states.State, changes *plans.Changes) string { return fmt.Sprintf( diff --git a/helper/resource/testing_import_state.go b/helper/resource/testing_import_state.go index c3e8580b1..d3029de87 100644 --- a/helper/resource/testing_import_state.go +++ b/helper/resource/testing_import_state.go @@ -62,10 +62,6 @@ func testStepImportState( return state, stepDiags.Err() } - // We will need access to the schemas in order to shim to the old-style - // testing API. - schemas := ctx.Schemas() - // The test step provides the resource address as a string, so we need // to parse it to get an addrs.AbsResourceAddress to pass in to the // import method. @@ -95,7 +91,7 @@ func testStepImportState( return state, stepDiags.Err() } - newState, err := shimNewState(importedState, schemas) + newState, err := shimNewState(importedState, step.providers) if err != nil { return nil, err } From f4fe6d671623f90689b2aa563b643d54fda6ffe2 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 10 Jan 2019 12:24:05 -0500 Subject: [PATCH 2/3] add tests with set hashes to the test provider These are representative of things that real-world providers use in tests. --- .../providers/test/resource_nested_test.go | 66 +++++++++++++++++-- 1 file changed, 59 insertions(+), 7 deletions(-) diff --git a/builtin/providers/test/resource_nested_test.go b/builtin/providers/test/resource_nested_test.go index 9237b52fc..24ac425f5 100644 --- a/builtin/providers/test/resource_nested_test.go +++ b/builtin/providers/test/resource_nested_test.go @@ -24,6 +24,14 @@ resource "test_resource_nested" "foo" { } } `), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + "test_resource_nested.foo", "nested.#", "1", + ), + resource.TestCheckResourceAttr( + "test_resource_nested.foo", "nested.1877647874.string", "val", + ), + ), }, }, }) @@ -31,7 +39,7 @@ resource "test_resource_nested" "foo" { func TestResourceNested_addRemove(t *testing.T) { var id string - checkFunc := func(s *terraform.State) error { + idCheck := func(s *terraform.State) error { root := s.ModuleByPath(addrs.RootModuleInstance) res := root.Resources["test_resource_nested.foo"] if res.Primary.ID == id { @@ -49,7 +57,17 @@ func TestResourceNested_addRemove(t *testing.T) { resource "test_resource_nested" "foo" { } `), - Check: checkFunc, + Check: resource.ComposeTestCheckFunc( + idCheck, + resource.TestCheckResourceAttr( + "test_resource_nested.foo", "nested.#", "0", + ), + // Checking for a count of 0 and a nonexistent count should + // now be the same operation. + resource.TestCheckNoResourceAttr( + "test_resource_nested.foo", "nested.#", + ), + ), }, resource.TestStep{ Config: strings.TrimSpace(` @@ -59,7 +77,12 @@ resource "test_resource_nested" "foo" { } } `), - Check: checkFunc, + Check: resource.ComposeTestCheckFunc( + idCheck, + resource.TestCheckResourceAttr( + "test_resource_nested.foo", "nested.1877647874.string", "val", + ), + ), }, resource.TestStep{ Config: strings.TrimSpace(` @@ -70,7 +93,15 @@ resource "test_resource_nested" "foo" { } } `), - Check: checkFunc, + Check: resource.ComposeTestCheckFunc( + idCheck, + resource.TestCheckResourceAttr( + "test_resource_nested.foo", "nested.1877647874.string", "val", + ), + resource.TestCheckResourceAttr( + "test_resource_nested.foo", "optional", "true", + ), + ), }, resource.TestStep{ Config: strings.TrimSpace(` @@ -80,7 +111,15 @@ resource "test_resource_nested" "foo" { } } `), - Check: checkFunc, + Check: resource.ComposeTestCheckFunc( + idCheck, + resource.TestCheckResourceAttr( + "test_resource_nested.foo", "nested.1877647874.string", "val", + ), + resource.TestCheckNoResourceAttr( + "test_resource_nested.foo", "optional", + ), + ), }, resource.TestStep{ Config: strings.TrimSpace(` @@ -91,14 +130,27 @@ resource "test_resource_nested" "foo" { } } `), - Check: checkFunc, + Check: resource.ComposeTestCheckFunc( + idCheck, + resource.TestCheckResourceAttr( + "test_resource_nested.foo", "nested.2994502535.string", "val", + ), + resource.TestCheckResourceAttr( + "test_resource_nested.foo", "nested.2994502535.optional", "true", + ), + ), }, resource.TestStep{ Config: strings.TrimSpace(` resource "test_resource_nested" "foo" { } `), - Check: checkFunc, + Check: resource.ComposeTestCheckFunc( + idCheck, + resource.TestCheckNoResourceAttr( + "test_resource_nested.foo", "nested.#", + ), + ), }, }, }) From 3e3802c36f14b682b61a75c9d63954d0c11f8863 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 10 Jan 2019 13:08:54 -0500 Subject: [PATCH 3/3] update existing test provider test --- builtin/providers/test/resource_nested_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/builtin/providers/test/resource_nested_test.go b/builtin/providers/test/resource_nested_test.go index 24ac425f5..00b4e0f28 100644 --- a/builtin/providers/test/resource_nested_test.go +++ b/builtin/providers/test/resource_nested_test.go @@ -187,14 +187,14 @@ resource "test_resource_nested" "foo" { got := rs.Primary.Attributes want := map[string]string{ - "nested.#": "2", - "nested.0.string": "a", - "nested.0.optional": "false", - "nested.0.nested_again.#": "1", - "nested.0.nested_again.0.string": "a", - "nested.1.string": "", - "nested.1.optional": "false", - "nested.1.nested_again.#": "0", + "nested.#": "2", + "nested.33842314.string": "a", + "nested.33842314.optional": "false", + "nested.33842314.nested_again.#": "1", + "nested.33842314.nested_again.936590934.string": "a", + "nested.140280279.string": "", + "nested.140280279.optional": "false", + "nested.140280279.nested_again.#": "0", } delete(got, "id") // it's random, so not useful for testing