From 0fb24c1a7ae187faad0d85952c708dea258f2c86 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 20 Feb 2017 14:58:24 -0500 Subject: [PATCH 1/4] Remove sleep-based concurrency from newVertex test Add a synchronization channel for the TestWalker_newVertex test, rather than using a sleep and running it multiple times. --- dag/walk_test.go | 69 ++++++++++++++++++++++++++++-------------------- 1 file changed, 41 insertions(+), 28 deletions(-) diff --git a/dag/walk_test.go b/dag/walk_test.go index 628ae6f64..76dd1f9e5 100644 --- a/dag/walk_test.go +++ b/dag/walk_test.go @@ -92,38 +92,51 @@ func TestWalker_error(t *testing.T) { } func TestWalker_newVertex(t *testing.T) { - // Run it a bunch of times since it is timing dependent - for i := 0; i < 50; i++ { - var g AcyclicGraph - g.Add(1) - g.Add(2) - g.Connect(BasicEdge(1, 2)) + var g AcyclicGraph + g.Add(1) + g.Add(2) + g.Connect(BasicEdge(1, 2)) - var order []interface{} - w := &Walker{Callback: walkCbRecord(&order)} - w.Update(&g) + // Record function + var order []interface{} + recordF := walkCbRecord(&order) + done2 := make(chan int) - // Wait a bit - time.Sleep(10 * time.Millisecond) - - // Update the graph - g.Add(3) - w.Update(&g) - - // Update the graph again but with the same vertex - g.Add(3) - w.Update(&g) - - // Wait - if err := w.Wait(); err != nil { - t.Fatalf("err: %s", err) + // Build a callback that notifies us when 2 has been walked + var w *Walker + cb := func(v Vertex) error { + if v == 2 { + defer func() { + close(done2) + }() } + return recordF(v) + } - // Check - expected := []interface{}{1, 2, 3} - if !reflect.DeepEqual(order, expected) { - t.Fatalf("bad: %#v", order) - } + // Add the initial vertices + w = &Walker{Callback: cb} + w.Update(&g) + + // if 2 has been visited, the walk is complete so far + <-done2 + + // Update the graph + g.Add(3) + w.Update(&g) + + // Update the graph again but with the same vertex + g.Add(3) + w.Update(&g) + + // Wait + if err := w.Wait(); err != nil { + t.Fatalf("err: %s", err) + } + + // Check + expected := []interface{}{1, 2, 3} + if !reflect.DeepEqual(order, expected) { + t.Fatalf("bad: %#v", order) } } From 7bf33c2a7f1624c2f990d56f9b19acaa3d6c9cdd Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 20 Feb 2017 15:07:09 -0500 Subject: [PATCH 2/4] Remove loop from TestWalker_removeVertex There's no timing dependent behavior here, since V1 must be visited before V2, Remove and Update must be called before V2 is visited. --- dag/walk_test.go | 60 ++++++++++++++++++++++-------------------------- 1 file changed, 27 insertions(+), 33 deletions(-) diff --git a/dag/walk_test.go b/dag/walk_test.go index 76dd1f9e5..ca33e1097 100644 --- a/dag/walk_test.go +++ b/dag/walk_test.go @@ -106,9 +106,7 @@ func TestWalker_newVertex(t *testing.T) { var w *Walker cb := func(v Vertex) error { if v == 2 { - defer func() { - close(done2) - }() + defer close(done2) } return recordF(v) } @@ -141,42 +139,38 @@ func TestWalker_newVertex(t *testing.T) { } func TestWalker_removeVertex(t *testing.T) { - // Run it a bunch of times since it is timing dependent - for i := 0; i < 50; i++ { - var g AcyclicGraph - g.Add(1) - g.Add(2) - g.Connect(BasicEdge(1, 2)) + var g AcyclicGraph + g.Add(1) + g.Add(2) + g.Connect(BasicEdge(1, 2)) - // Record function - var order []interface{} - recordF := walkCbRecord(&order) + // Record function + var order []interface{} + recordF := walkCbRecord(&order) - // Build a callback that delays until we close a channel - var w *Walker - cb := func(v Vertex) error { - if v == 1 { - g.Remove(2) - w.Update(&g) - } - - return recordF(v) + var w *Walker + cb := func(v Vertex) error { + if v == 1 { + g.Remove(2) + w.Update(&g) } - // Add the initial vertices - w = &Walker{Callback: cb} - w.Update(&g) + return recordF(v) + } - // Wait - if err := w.Wait(); err != nil { - t.Fatalf("err: %s", err) - } + // Add the initial vertices + w = &Walker{Callback: cb} + w.Update(&g) - // Check - expected := []interface{}{1} - if !reflect.DeepEqual(order, expected) { - t.Fatalf("bad: %#v", order) - } + // Wait + if err := w.Wait(); err != nil { + t.Fatalf("err: %s", err) + } + + // Check + expected := []interface{}{1} + if !reflect.DeepEqual(order, expected) { + t.Fatalf("bad: %#v", order) } } From d01b0b064798dc1373fd0081965f4cc992dc7cd1 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 20 Feb 2017 15:18:25 -0500 Subject: [PATCH 3/4] Remove intermittent failure from newEdge test Because the vertex visit was record after the Update call, Updated vertices may have been visited before the visit was recorded, causing occasional test failures. The order is now deterministic, and we can remove the brute-force loop. --- dag/walk_test.go | 59 ++++++++++++++++++++++++------------------------ 1 file changed, 29 insertions(+), 30 deletions(-) diff --git a/dag/walk_test.go b/dag/walk_test.go index ca33e1097..6c1f24942 100644 --- a/dag/walk_test.go +++ b/dag/walk_test.go @@ -175,43 +175,42 @@ func TestWalker_removeVertex(t *testing.T) { } func TestWalker_newEdge(t *testing.T) { - // Run it a bunch of times since it is timing dependent - for i := 0; i < 50; i++ { - var g AcyclicGraph - g.Add(1) - g.Add(2) - g.Connect(BasicEdge(1, 2)) + var g AcyclicGraph + g.Add(1) + g.Add(2) + g.Connect(BasicEdge(1, 2)) - // Record function - var order []interface{} - recordF := walkCbRecord(&order) + // Record function + var order []interface{} + recordF := walkCbRecord(&order) - // Build a callback that delays until we close a channel - var w *Walker - cb := func(v Vertex) error { - if v == 1 { - g.Add(3) - g.Connect(BasicEdge(3, 2)) - w.Update(&g) - } + var w *Walker + cb := func(v Vertex) error { + // record where we are first, otherwise the Updated vertex may get + // walked before the first visit. + err := recordF(v) - return recordF(v) + if v == 1 { + g.Add(3) + g.Connect(BasicEdge(3, 2)) + w.Update(&g) } + return err + } - // Add the initial vertices - w = &Walker{Callback: cb} - w.Update(&g) + // Add the initial vertices + w = &Walker{Callback: cb} + w.Update(&g) - // Wait - if err := w.Wait(); err != nil { - t.Fatalf("err: %s", err) - } + // Wait + if err := w.Wait(); err != nil { + t.Fatalf("err: %s", err) + } - // Check - expected := []interface{}{1, 3, 2} - if !reflect.DeepEqual(order, expected) { - t.Fatalf("bad: %#v", order) - } + // Check + expected := []interface{}{1, 3, 2} + if !reflect.DeepEqual(order, expected) { + t.Fatalf("bad: %#v", order) } } From bfa6ab46176b19e1a667bec68184dcb2c27f0845 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 20 Feb 2017 15:36:15 -0500 Subject: [PATCH 4/4] Fix removeEdge test failures The removeEdge test could fail intermittently with the wrong order. The precondition of a 1->2->3 order wasn't met, because there was no edge from 1->3, so 3->1->2 was also a valid ordering. The other failure was a bookkeeping error, were the recorded order may not match the visited order. What happened in this case was the gateCh was closed by V2, allowing V3 to run which could beat V2 to recording its visit. Now the visit is recorded as part of the vertex walk, and the gate is released as the final operation. The order is deterministic now, so remove the brute-force test loop. --- dag/walk_test.go | 100 +++++++++++++++++++++++------------------------ 1 file changed, 50 insertions(+), 50 deletions(-) diff --git a/dag/walk_test.go b/dag/walk_test.go index 6c1f24942..9095d7189 100644 --- a/dag/walk_test.go +++ b/dag/walk_test.go @@ -215,63 +215,63 @@ func TestWalker_newEdge(t *testing.T) { } func TestWalker_removeEdge(t *testing.T) { - // Run it a bunch of times since it is timing dependent - for i := 0; i < 50; i++ { - var g AcyclicGraph - g.Add(1) - g.Add(2) - g.Add(3) - g.Connect(BasicEdge(1, 2)) - g.Connect(BasicEdge(3, 2)) + var g AcyclicGraph + g.Add(1) + g.Add(2) + g.Add(3) + g.Connect(BasicEdge(1, 2)) + g.Connect(BasicEdge(1, 3)) + g.Connect(BasicEdge(3, 2)) - // Record function - var order []interface{} - recordF := walkCbRecord(&order) + // Record function + var order []interface{} + recordF := walkCbRecord(&order) - // The way this works is that our original graph forces - // the order of 1 => 3 => 2. During the execution of 1, we - // remove the edge forcing 3 before 2. Then, during the execution - // of 3, we wait on a channel that is only closed by 2, implicitly - // forcing 2 before 3 via the callback (and not the graph). If - // 2 cannot execute before 3 (edge removal is non-functional), then - // this test will timeout. - var w *Walker - gateCh := make(chan struct{}) - cb := func(v Vertex) error { - if v == 1 { - g.RemoveEdge(BasicEdge(3, 2)) - w.Update(&g) + // The way this works is that our original graph forces + // the order of 1 => 3 => 2. During the execution of 1, we + // remove the edge forcing 3 before 2. Then, during the execution + // of 3, we wait on a channel that is only closed by 2, implicitly + // forcing 2 before 3 via the callback (and not the graph). If + // 2 cannot execute before 3 (edge removal is non-functional), then + // this test will timeout. + var w *Walker + gateCh := make(chan struct{}) + cb := func(v Vertex) error { + switch v { + case 1: + g.RemoveEdge(BasicEdge(3, 2)) + w.Update(&g) + + case 2: + // this visit isn't completed until we've recorded it + // Once the visit is official, we can then close the gate to + // let 3 continue. + defer close(gateCh) + + case 3: + select { + case <-gateCh: + case <-time.After(50 * time.Millisecond): + return fmt.Errorf("timeout 3 waiting for 2") } - - if v == 2 { - close(gateCh) - } - - if v == 3 { - select { - case <-gateCh: - case <-time.After(50 * time.Millisecond): - return fmt.Errorf("timeout 3 waiting for 2") - } - } - - return recordF(v) } - // Add the initial vertices - w = &Walker{Callback: cb} - w.Update(&g) + return recordF(v) + } - // Wait - if err := w.Wait(); err != nil { - t.Fatalf("err: %s", err) - } + // Add the initial vertices + w = &Walker{Callback: cb} + w.Update(&g) - // Check - expected := []interface{}{1, 2, 3} - if !reflect.DeepEqual(order, expected) { - t.Fatalf("bad: %#v", order) - } + // Wait + if err := w.Wait(); err != nil { + t.Fatalf("err: %s", err) + } + + // Check + expected := []interface{}{1, 2, 3} + if !reflect.DeepEqual(order, expected) { + t.Fatalf("bad: %#v", order) } }