From 17c1055a0e5685e6f8e23014bd2ca9e022255775 Mon Sep 17 00:00:00 2001 From: Peter Tseng Date: Tue, 27 Oct 2015 03:34:48 -0700 Subject: [PATCH 1/2] react: factor out assertCellValue The motivation is twofold: First, to reduce repetition in the code. Second, to provide the user the ability to see the expected value of the cell versus the observed value so the user has a better idea of what went wrong. --- react/react_test.go | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/react/react_test.go b/react/react_test.go index a9cfbaf09..8e102dbfb 100644 --- a/react/react_test.go +++ b/react/react_test.go @@ -21,17 +21,20 @@ func TestTestVersion(t *testing.T) { } } +func assertCellValue(t *testing.T, c Cell, expected int, explanation string) { + observed := c.Value() + if observed != expected { + t.Fatalf("%s: expected %d, got %d", explanation, expected, observed) + } +} + // Setting the value of an input cell changes the observable Value() func TestSetInput(t *testing.T) { r := New() i := r.CreateInput(1) - if i.Value() != 1 { - t.Fatalf("i.Value() doesn't match initial value") - } + assertCellValue(t, i, 1, "i.Value() doesn't match initial value") i.SetValue(2) - if i.Value() != 2 { - t.Fatalf("i.Value() doesn't match changed value") - } + assertCellValue(t, i, 2, "i.Value() doesn't match changed value") } // The value of a compute cell is determined by the value of the dependencies. @@ -39,13 +42,9 @@ func TestBasicCompute(t *testing.T) { r := New() i := r.CreateInput(1) c := r.CreateCompute1(i, func(v int) int { return v + 1 }) - if c.Value() != 2 { - t.Fatalf("c.Value() isn't properly computed based on initial input cell value") - } + assertCellValue(t, c, 2, "c.Value() isn't properly computed based on initial input cell value") i.SetValue(2) - if c.Value() != 3 { - t.Fatalf("c.Value() isn't properly computed based on changed input cell value") - } + assertCellValue(t, c, 3, "c.Value() isn't properly computed based on changed input cell value") } // Compute cells can depend on other compute cells. @@ -55,13 +54,9 @@ func TestTopology(t *testing.T) { c1 := r.CreateCompute1(i, func(v int) int { return v + 1 }) c2 := r.CreateCompute1(i, func(v int) int { return v - 1 }) c3 := r.CreateCompute2(c1, c2, func(v1, v2 int) int { return v1 * v2 }) - if c3.Value() != 0 { - t.Fatalf("c3.Value() isn't properly computed based on initial input cell value") - } + assertCellValue(t, c3, 0, "c3.Value() isn't properly computed based on initial input cell value") i.SetValue(3) - if c3.Value() != 8 { - t.Fatalf("c3.Value() isn't properly computed based on changed input cell value") - } + assertCellValue(t, c3, 8, "c3.Value() isn't properly computed based on changed input cell value") } // Compute cells can have callbacks. From c7616dc42fbef0eae51b0ac5792ef7916770d868 Mon Sep 17 00:00:00 2001 From: Peter Tseng Date: Tue, 27 Oct 2015 03:56:46 -0700 Subject: [PATCH 2/2] react: Add tests with new topologies The added toplogies are all topologies that are not being tested before: * compute2 depending directly on inputs (instead of on compute1). Note that this also tests having multiple inputs, something that was not tested before. * compute1 depending on other compute1 for value. Note that the multiple dep changes test has a compute1 depending on compute1... but it only tests for the callback of a dependent compute2 and doesn't check the value of the dependent compute1, so this test is still new. * compute2 depending on other compute2. Why should we care? It is possible that the limited range of topologies in the existing tests will drive some solutions to work only for those specific topologies. Before this commit, the minimal non-generic solution that would pass tests is "when an input is updated, update compute1 cells depending on that input, update compute2 cells depending on those compute1 cells". Again note that the multiple dep changes test has a compute1 -> compute1 connection but the value is never checked, so the test still passes despite this minimal solution not updating it. Knowing this, it could be somewhat tempting to implement such a solution instead of the generic solution. I believe the new tests make clearer that a solution should be generic across all possible topologies. Clearly we can never *ensure* that with just tests, but I think this gets us closer than we were before. Solutions will be more likely to take it into account, and reviewers are freed from having to explain about topology limitations and can focus attention on other areas. Test version is bumped for this. --- react/example.go | 2 +- react/react_test.go | 67 +++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 63 insertions(+), 6 deletions(-) diff --git a/react/example.go b/react/example.go index aabdd4832..861a8f37c 100644 --- a/react/example.go +++ b/react/example.go @@ -1,6 +1,6 @@ package react -const TestVersion = 1 +const TestVersion = 2 type reactor struct { cells []*cell diff --git a/react/react_test.go b/react/react_test.go index 8e102dbfb..1f438558f 100644 --- a/react/react_test.go +++ b/react/react_test.go @@ -8,7 +8,10 @@ import "testing" // Also define an exported TestVersion with a value that matches // the internal testVersion here. -const testVersion = 1 +const testVersion = 2 + +// Retired: +// 1 afa5c1278857457403a30479663d26d4e1c8c496 // This is a compile time check to see if you've properly implemented New(). var _ Reactor = New() @@ -37,8 +40,8 @@ func TestSetInput(t *testing.T) { assertCellValue(t, i, 2, "i.Value() doesn't match changed value") } -// The value of a compute cell is determined by the value of the dependencies. -func TestBasicCompute(t *testing.T) { +// The value of a compute 1 cell is determined by the value of the dependencies. +func TestBasicCompute1(t *testing.T) { r := New() i := r.CreateInput(1) c := r.CreateCompute1(i, func(v int) int { return v + 1 }) @@ -47,8 +50,21 @@ func TestBasicCompute(t *testing.T) { assertCellValue(t, c, 3, "c.Value() isn't properly computed based on changed input cell value") } -// Compute cells can depend on other compute cells. -func TestTopology(t *testing.T) { +// The value of a compute 2 cell is determined by the value of the dependencies. +func TestBasicCompute2(t *testing.T) { + r := New() + i1 := r.CreateInput(1) + i2 := r.CreateInput(2) + c := r.CreateCompute2(i1, i2, func(v1, v2 int) int { return v1 | v2 }) + assertCellValue(t, c, 3, "c.Value() isn't properly computed based on initial input cell values") + i1.SetValue(4) + assertCellValue(t, c, 6, "c.Value() isn't properly computed when first input cell value changes") + i2.SetValue(8) + assertCellValue(t, c, 12, "c.Value() isn't properly computed when second input cell value changes") +} + +// Compute 2 cells can depend on compute 1 cells. +func TestCompute2Diamond(t *testing.T) { r := New() i := r.CreateInput(1) c1 := r.CreateCompute1(i, func(v int) int { return v + 1 }) @@ -59,6 +75,47 @@ func TestTopology(t *testing.T) { assertCellValue(t, c3, 8, "c3.Value() isn't properly computed based on changed input cell value") } +// Compute 1 cells can depend on other compute 1 cells. +func TestCompute1Chain(t *testing.T) { + r := New() + inp := r.CreateInput(1) + var c Cell = inp + for i := 2; i <= 8; i++ { + // must save current value of loop variable i for correct behavior. + // compute function has to use digitToAdd not i. + digitToAdd := i + c = r.CreateCompute1(c, func(v int) int { return v*10 + digitToAdd }) + } + assertCellValue(t, c, 12345678, "c.Value() isn't properly computed based on initial input cell value") + inp.SetValue(9) + assertCellValue(t, c, 92345678, "c.Value() isn't properly computed based on changed input cell value") +} + +// Compute 2 cells can depend on other compute 2 cells. +func TestCompute2Tree(t *testing.T) { + r := New() + ins := make([]InputCell, 3) + for i, v := range []int{1, 10, 100} { + ins[i] = r.CreateInput(v) + } + + add := func(v1, v2 int) int { return v1 + v2 } + + firstLevel := make([]ComputeCell, 2) + for i := 0; i < 2; i++ { + firstLevel[i] = r.CreateCompute2(ins[i], ins[i+1], add) + } + + output := r.CreateCompute2(firstLevel[0], firstLevel[1], add) + assertCellValue(t, output, 121, "output.Value() isn't properly computed based on initial input cell values") + + for i := 0; i < 3; i++ { + ins[i].SetValue(ins[i].Value() * 2) + } + + assertCellValue(t, output, 242, "output.Value() isn't properly computed based on changed input cell values") +} + // Compute cells can have callbacks. func TestBasicCallback(t *testing.T) { r := New()