Merge pull request #12498 from hashicorp/jbardin/test-reset

Add schema.Provider.TestReset to reset StopContext between tests

helper/resource/testing.go

@@ -22,6 +22,13 @@ import (
 
 const TestEnvVar = "TF_ACC"
 
+// TestProvider can be implemented by any ResourceProvider to provide custom
+// reset functionality at the start of an acceptance test.
+// The helper/schema Provider implements this interface.
+type TestProvider interface {
+	TestReset() error
+}
+
 // TestCheckFunc is the callback type used with acceptance tests to check
 // the state of a resource. The state passed in is the latest state known,
 // or in the case of being after a destroy, it is the last known state when
@@ -216,13 +223,9 @@ func Test(t TestT, c TestCase) {
 		c.PreCheck()
 	}
 
-	// Build our context options that we can
-	ctxProviders := c.ProviderFactories
-	if ctxProviders == nil {
-		ctxProviders = make(map[string]terraform.ResourceProviderFactory)
-		for k, p := range c.Providers {
-			ctxProviders[k] = terraform.ResourceProviderFactoryFixed(p)
-		}
+	ctxProviders, err := testProviderFactories(c)
+	if err != nil {
+		t.Fatal(err)
 	}
 	opts := terraform.ContextOpts{Providers: ctxProviders}
 
@@ -333,6 +336,43 @@ func Test(t TestT, c TestCase) {
 	}
 }
 
+// testProviderFactories is a helper to build the ResourceProviderFactory map
+// 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 testProviderFactories(c TestCase) (map[string]terraform.ResourceProviderFactory, error) {
+	ctxProviders := make(map[string]terraform.ResourceProviderFactory)
+
+	// add any fixed providers
+	for k, p := range c.Providers {
+		ctxProviders[k] = terraform.ResourceProviderFactoryFixed(p)
+	}
+
+	// call any factory functions and store the result.
+	for k, pf := range c.ProviderFactories {
+		p, err := pf()
+		ctxProviders[k] = func() (terraform.ResourceProvider, error) {
+			return p, err
+		}
+	}
+
+	// reset the providers if needed
+	for k, pf := range ctxProviders {
+		// we can ignore any errors here, if we don't have a provider to reset
+		// the error will be handled later
+		p, _ := pf()
+		if p, ok := p.(TestProvider); ok {
+			err := p.TestReset()
+			if err != nil {
+				return nil, fmt.Errorf("[ERROR] failed to reset provider %q: %s", k, err)
+			}
+		}
+	}
+
+	return ctxProviders, 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.

helper/resource/testing_test.go

@@ -4,7 +4,9 @@ import (
 	"errors"
 	"fmt"
 	"os"
+	"regexp"
 	"strings"
+	"sync"
 	"sync/atomic"
 	"testing"
 
@@ -25,8 +27,26 @@ func init() {
 	}
 }
 
+// wrap the mock provider to implement TestProvider
+type resetProvider struct {
+	*terraform.MockResourceProvider
+	mu              sync.Mutex
+	TestResetCalled bool
+	TestResetError  error
+}
+
+func (p *resetProvider) TestReset() error {
+	p.mu.Lock()
+	defer p.mu.Unlock()
+	p.TestResetCalled = true
+	return p.TestResetError
+}
+
 func TestTest(t *testing.T) {
-	mp := testProvider()
+	mp := &resetProvider{
+		MockResourceProvider: testProvider(),
+	}
+
 	mp.DiffReturn = nil
 
 	mp.ApplyFn = func(
@@ -95,6 +115,9 @@ func TestTest(t *testing.T) {
 	if !checkDestroy {
 		t.Fatal("didn't call check for destroy")
 	}
+	if !mp.TestResetCalled {
+		t.Fatal("didn't call TestReset")
+	}
 }
 
 func TestTest_idRefresh(t *testing.T) {
@@ -355,6 +378,53 @@ func TestTest_stepError(t *testing.T) {
 	}
 }
 
+func TestTest_factoryError(t *testing.T) {
+	resourceFactoryError := fmt.Errorf("resource factory error")
+
+	factory := func() (terraform.ResourceProvider, error) {
+		return nil, resourceFactoryError
+	}
+
+	mt := new(mockT)
+	Test(mt, TestCase{
+		ProviderFactories: map[string]terraform.ResourceProviderFactory{
+			"test": factory,
+		},
+		Steps: []TestStep{
+			TestStep{
+				ExpectError: regexp.MustCompile("resource factory error"),
+			},
+		},
+	})
+
+	if !mt.failed() {
+		t.Fatal("test should've failed")
+	}
+}
+
+func TestTest_resetError(t *testing.T) {
+	mp := &resetProvider{
+		MockResourceProvider: testProvider(),
+		TestResetError:       fmt.Errorf("provider reset error"),
+	}
+
+	mt := new(mockT)
+	Test(mt, TestCase{
+		Providers: map[string]terraform.ResourceProvider{
+			"test": mp,
+		},
+		Steps: []TestStep{
+			TestStep{
+				ExpectError: regexp.MustCompile("provider reset error"),
+			},
+		},
+	})
+
+	if !mt.failed() {
+		t.Fatal("test should've failed")
+	}
+}
+
 func TestComposeAggregateTestCheckFunc(t *testing.T) {
 	check1 := func(s *terraform.State) error {
 		return errors.New("Error 1")

helper/schema/provider.go

@@ -50,8 +50,15 @@ type Provider struct {
 	// See the ConfigureFunc documentation for more information.
 	ConfigureFunc ConfigureFunc
 
+	// MetaReset is called by TestReset to reset any state stored in the meta
+	// interface.  This is especially important if the StopContext is stored by
+	// the provider.
+	MetaReset func() error
+
 	meta interface{}
 
+	// a mutex is required because TestReset can directly repalce the stopCtx
+	stopMu        sync.Mutex
 	stopCtx       context.Context
 	stopCtxCancel context.CancelFunc
 	stopOnce      sync.Once
@@ -124,20 +131,43 @@ func (p *Provider) Stopped() bool {
 // StopCh returns a channel that is closed once the provider is stopped.
 func (p *Provider) StopContext() context.Context {
 	p.stopOnce.Do(p.stopInit)
+
+	p.stopMu.Lock()
+	defer p.stopMu.Unlock()
+
 	return p.stopCtx
 }
 
 func (p *Provider) stopInit() {
+	p.stopMu.Lock()
+	defer p.stopMu.Unlock()
+
 	p.stopCtx, p.stopCtxCancel = context.WithCancel(context.Background())
 }
 
 // Stop implementation of terraform.ResourceProvider interface.
 func (p *Provider) Stop() error {
 	p.stopOnce.Do(p.stopInit)
+
+	p.stopMu.Lock()
+	defer p.stopMu.Unlock()
+
 	p.stopCtxCancel()
 	return nil
 }
 
+// TestReset resets any state stored in the Provider, and will call TestReset
+// on Meta if it implements the TestProvider interface.
+// This may be used to reset the schema.Provider at the start of a test, and is
+// automatically called by resource.Test.
+func (p *Provider) TestReset() error {
+	p.stopInit()
+	if p.MetaReset != nil {
+		return p.MetaReset()
+	}
+	return nil
+}
+
 // Input implementation of terraform.ResourceProvider interface.
 func (p *Provider) Input(
 	input terraform.UIInput,

helper/schema/provider_test.go

@@ -381,3 +381,29 @@ func TestProviderStop_stopFirst(t *testing.T) {
 		t.Fatal("should be stopped")
 	}
 }
+
+func TestProviderReset(t *testing.T) {
+	var p Provider
+	stopCtx := p.StopContext()
+	p.MetaReset = func() error {
+		stopCtx = p.StopContext()
+		return nil
+	}
+
+	// cancel the current context
+	p.Stop()
+
+	if err := p.TestReset(); err != nil {
+		t.Fatal(err)
+	}
+
+	// the first context should have been replaced
+	if err := stopCtx.Err(); err != nil {
+		t.Fatal(err)
+	}
+
+	// we should not get a canceled context here either
+	if err := p.StopContext().Err(); err != nil {
+		t.Fatal(err)
+	}
+}

terraform/context.go

@@ -781,15 +781,14 @@ func (c *Context) walk(
 	}
 
 	// Watch for a stop so we can call the provider Stop() API.
-	doneCh := make(chan struct{})
-	stopCh := c.runContext.Done()
-	go c.watchStop(walker, doneCh, stopCh)
+	watchStop, watchWait := c.watchStop(walker)
 
 	// Walk the real graph, this will block until it completes
 	realErr := graph.Walk(walker)
 
-	// Close the done channel so the watcher stops
-	close(doneCh)
+	// Close the channel so the watcher stops, and wait for it to return.
+	close(watchStop)
+	<-watchWait
 
 	// If we have a shadow graph and we interrupted the real graph, then
 	// we just close the shadow and never verify it. It is non-trivial to
@@ -878,52 +877,74 @@ func (c *Context) walk(
 	return walker, realErr
 }
 
-func (c *Context) watchStop(walker *ContextGraphWalker, doneCh, stopCh <-chan struct{}) {
-	// Wait for a stop or completion
-	select {
-	case <-stopCh:
-		// Stop was triggered. Fall out of the select
-	case <-doneCh:
-		// Done, just exit completely
-		return
-	}
+// watchStop immediately returns a `stop` and a `wait` chan after dispatching
+// the watchStop goroutine. This will watch the runContext for cancellation and
+// stop the providers accordingly.  When the watch is no longer needed, the
+// `stop` chan should be closed before waiting on the `wait` chan.
+// The `wait` chan is important, because without synchronizing with the end of
+// the watchStop goroutine, the runContext may also be closed during the select
+// incorrectly causing providers to be stopped. Even if the graph walk is done
+// at that point, stopping a provider permanently cancels its StopContext which
+// can cause later actions to fail.
+func (c *Context) watchStop(walker *ContextGraphWalker) (chan struct{}, <-chan struct{}) {
+	stop := make(chan struct{})
+	wait := make(chan struct{})
 
-	// If we're here, we're stopped, trigger the call.
+	// get the runContext cancellation channel now, because releaseRun will
+	// write to the runContext field.
+	done := c.runContext.Done()
 
-	{
-		// Copy the providers so that a misbehaved blocking Stop doesn't
-		// completely hang Terraform.
-		walker.providerLock.Lock()
-		ps := make([]ResourceProvider, 0, len(walker.providerCache))
-		for _, p := range walker.providerCache {
-			ps = append(ps, p)
+	go func() {
+		defer close(wait)
+		// Wait for a stop or completion
+		select {
+		case <-done:
+			// done means the context was canceled, so we need to try and stop
+			// providers.
+		case <-stop:
+			// our own stop channel was closed.
+			return
 		}
-		defer walker.providerLock.Unlock()
 
-		for _, p := range ps {
-			// We ignore the error for now since there isn't any reasonable
-			// action to take if there is an error here, since the stop is still
-			// advisory: Terraform will exit once the graph node completes.
-			p.Stop()
-		}
-	}
+		// If we're here, we're stopped, trigger the call.
 
-	{
-		// Call stop on all the provisioners
-		walker.provisionerLock.Lock()
-		ps := make([]ResourceProvisioner, 0, len(walker.provisionerCache))
-		for _, p := range walker.provisionerCache {
-			ps = append(ps, p)
-		}
-		defer walker.provisionerLock.Unlock()
+		{
+			// Copy the providers so that a misbehaved blocking Stop doesn't
+			// completely hang Terraform.
+			walker.providerLock.Lock()
+			ps := make([]ResourceProvider, 0, len(walker.providerCache))
+			for _, p := range walker.providerCache {
+				ps = append(ps, p)
+			}
+			defer walker.providerLock.Unlock()
 
-		for _, p := range ps {
-			// We ignore the error for now since there isn't any reasonable
-			// action to take if there is an error here, since the stop is still
-			// advisory: Terraform will exit once the graph node completes.
-			p.Stop()
+			for _, p := range ps {
+				// We ignore the error for now since there isn't any reasonable
+				// action to take if there is an error here, since the stop is still
+				// advisory: Terraform will exit once the graph node completes.
+				p.Stop()
+			}
 		}
-	}
+
+		{
+			// Call stop on all the provisioners
+			walker.provisionerLock.Lock()
+			ps := make([]ResourceProvisioner, 0, len(walker.provisionerCache))
+			for _, p := range walker.provisionerCache {
+				ps = append(ps, p)
+			}
+			defer walker.provisionerLock.Unlock()
+
+			for _, p := range ps {
+				// We ignore the error for now since there isn't any reasonable
+				// action to take if there is an error here, since the stop is still
+				// advisory: Terraform will exit once the graph node completes.
+				p.Stop()
+			}
+		}
+	}()
+
+	return stop, wait
 }
 
 // parseVariableAsHCL parses the value of a single variable as would have been specified