From cba592d54f83d0b98f85aa2005d6227f33f660af Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 20 Dec 2017 09:18:38 -0500 Subject: [PATCH 1/3] minor race issue in mockResourceProvider The interrupt tests for providers no longer check for the condition during the diff operation. defer the lock so other test's DiffFns don't need to be as carefull locking themselves. --- terraform/resource_provider_mock.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/terraform/resource_provider_mock.go b/terraform/resource_provider_mock.go index 9131f0f5f..4000e3d21 100644 --- a/terraform/resource_provider_mock.go +++ b/terraform/resource_provider_mock.go @@ -196,13 +196,13 @@ func (p *MockResourceProvider) Diff( info *InstanceInfo, state *InstanceState, desired *ResourceConfig) (*InstanceDiff, error) { - p.Lock() + defer p.Unlock() + p.DiffCalled = true p.DiffInfo = info p.DiffState = state p.DiffDesired = desired - p.Unlock() if p.DiffFn != nil { return p.DiffFn(info, state, desired) From e63a3474d5a4aebb014d0b3e31d1bdfcbef7130d Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 20 Dec 2017 15:09:11 -0500 Subject: [PATCH 2/3] kill the flag error writer after 2 seconds There's no point in trying to track these, they're lost after each test. Kill them after a short delay so we don't have goroutines from every single command test to wade through if we have a stack dump. --- command/meta.go | 28 +++++++--------------------- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/command/meta.go b/command/meta.go index 27f7765f9..f5be9a73e 100644 --- a/command/meta.go +++ b/command/meta.go @@ -160,13 +160,6 @@ type Meta struct { forceInitCopy bool reconfigure bool - // errWriter is the write side of a pipe for the FlagSet output. We need to - // keep track of this to close previous pipes between tests. Normal - // operation never needs to close this. - errWriter *io.PipeWriter - // done chan to wait for the scanner goroutine - errScannerDone chan struct{} - // Used with the import command to allow import of state when no matching config exists. allowMissingConfig bool } @@ -339,23 +332,16 @@ func (m *Meta) flagSet(n string) *flag.FlagSet { // This is kind of a hack, but it does the job. Basically: create // a pipe, use a scanner to break it into lines, and output each line // to the UI. Do this forever. - - // If a previous pipe exists, we need to close that first. - // This should only happen in testing. - if m.errWriter != nil { - m.errWriter.Close() - } - - if m.errScannerDone != nil { - <-m.errScannerDone - } - errR, errW := io.Pipe() errScanner := bufio.NewScanner(errR) - m.errWriter = errW - m.errScannerDone = make(chan struct{}) go func() { - defer close(m.errScannerDone) + // This only needs to be alive long enough to write the help info if + // there is a flag error. Kill the scanner after a short duriation to + // prevent these from accumulating during tests, and cluttering up the + // stack traces. + time.AfterFunc(2*time.Second, func() { + errW.Close() + }) for errScanner.Scan() { m.Ui.Error(errScanner.Text()) } From d76482cd89928af77384c5a258c0c972021381aa Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 20 Dec 2017 15:51:02 -0500 Subject: [PATCH 3/3] don't try to interrupt diff in shutdown test Rather than relying on interrupting Diff, just make sure Stop was called on the provider. The DiffFn is protected by a mutex in the mock provider, which means that the tests can't rely on concurent calls to diff working. --- command/plan_test.go | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/command/plan_test.go b/command/plan_test.go index 04390ab5b..3e97fdd47 100644 --- a/command/plan_test.go +++ b/command/plan_test.go @@ -7,7 +7,9 @@ import ( "path/filepath" "reflect" "strings" + "sync" "testing" + "time" "github.com/hashicorp/terraform/helper/copy" "github.com/hashicorp/terraform/terraform" @@ -832,8 +834,7 @@ func TestPlan_detailedExitcode_emptyDiff(t *testing.T) { } func TestPlan_shutdown(t *testing.T) { - cancelled := false - stopped := make(chan struct{}) + cancelled := make(chan struct{}) shutdownCh := make(chan struct{}) p := testProvider() @@ -847,20 +848,20 @@ func TestPlan_shutdown(t *testing.T) { } p.StopFn = func() error { - close(stopped) - cancelled = true + close(cancelled) return nil } + var once sync.Once + p.DiffFn = func( *terraform.InstanceInfo, *terraform.InstanceState, *terraform.ResourceConfig) (*terraform.InstanceDiff, error) { - if !cancelled { + once.Do(func() { shutdownCh <- struct{}{} - <-stopped - } + }) return &terraform.InstanceDiff{ Attributes: map[string]*terraform.ResourceAttrDiff{ @@ -875,7 +876,9 @@ func TestPlan_shutdown(t *testing.T) { t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String()) } - if !cancelled { + select { + case <-cancelled: + case <-time.After(5 * time.Second): t.Fatal("command not cancelled") } }