From 90a0286909835a4ee2d374f692072aae43ee23c1 Mon Sep 17 00:00:00 2001 From: Peter Bourgon Date: Mon, 29 Jun 2015 15:20:59 +0200 Subject: [PATCH] Refactor tests to have appropriate packages By default, tests should be in package pkg_test. If they need to test package internals, they can be in package pkg, but then should carry a suffix of foo_internal_test.go. This changeset enforces that idiom across the codebase, and adds a check to the linter to make sure it remains. Also, some fixes to comments. --- bin/lint | 28 +++ experimental/_integration/easy_test.go | 2 +- report/diff_test.go | 21 -- report/merge.go | 15 +- report/merge_test.go | 181 +++++++++--------- test/diff.go | 2 +- ...tor_test.go => collector_internal_test.go} | 0 7 files changed, 128 insertions(+), 121 deletions(-) delete mode 100644 report/diff_test.go rename xfer/{collector_test.go => collector_internal_test.go} (100%) diff --git a/bin/lint b/bin/lint index a4102bfe30..f3a2560f89 100755 --- a/bin/lint +++ b/bin/lint @@ -25,6 +25,30 @@ function spell_check { return $lint_result } +function test_mismatch { + filename="$1" + package=$(grep '^package ' $filename | awk '{print $2}') + local lint_result=0 + + if [[ $package == "main" ]]; then + continue # in package main, all bets are off + fi + + if [[ $filename == *"_internal_test.go" ]]; then + if [[ $package == *"_test" ]]; then + lint_result=1 + echo "${filename}: should not be part of a _test package" + fi + else + if [[ ! $package == *"_test" ]]; then + lint_result=1 + echo "${filename}: should be part of a _test package" + fi + fi + + return $lint_result +} + function lint_go { filename="$1" local lint_result=0 @@ -81,6 +105,10 @@ function lint { ;; esac + if [[ $filename == *"_test.go" ]]; then + test_mismatch "${filename}" || lint_result=1 + fi + spell_check "${filename}" || lint_result=1 return $lint_result diff --git a/experimental/_integration/easy_test.go b/experimental/_integration/easy_test.go index 5e4736d98c..167fc2beee 100644 --- a/experimental/_integration/easy_test.go +++ b/experimental/_integration/easy_test.go @@ -1,4 +1,4 @@ -package integration +package integration_test import ( "encoding/json" diff --git a/report/diff_test.go b/report/diff_test.go deleted file mode 100644 index 88d8640cce..0000000000 --- a/report/diff_test.go +++ /dev/null @@ -1,21 +0,0 @@ -package report_test - -import ( - "github.com/davecgh/go-spew/spew" - "github.com/pmezard/go-difflib/difflib" -) - -func init() { - spew.Config.SortKeys = true // :\ -} - -func diff(want, have interface{}) string { - text, _ := difflib.GetUnifiedDiffString(difflib.UnifiedDiff{ - A: difflib.SplitLines(spew.Sdump(want)), - B: difflib.SplitLines(spew.Sdump(have)), - FromFile: "want", - ToFile: "have", - Context: 5, - }) - return "\n" + text -} diff --git a/report/merge.go b/report/merge.go index df7de7b58c..6c1e06d75e 100644 --- a/report/merge.go +++ b/report/merge.go @@ -1,8 +1,7 @@ package report -// Merge() functions for all topology datatypes. -// The general semantics are that the receiver is modified, and what's merged -// in isn't. +// Merge functions for all topology datatypes. The general semantics are that +// the receiver is modified, and what's merged in isn't. // Merge merges another Report into the receiver. func (r *Report) Merge(other Report) { @@ -38,9 +37,9 @@ func (m *NodeMetadatas) Merge(other NodeMetadatas) { } } -// Merge merges another EdgeMetadatas into the receiver. -// If other is from another probe this is the union of both metadatas. Keys -// present in both are summed. +// Merge merges another EdgeMetadatas into the receiver. If other is from +// another probe this is the union of both metadatas. Keys present in both are +// summed. func (e *EdgeMetadatas) Merge(other EdgeMetadatas) { for id, edgemeta := range other { local := (*e)[id] @@ -65,8 +64,8 @@ func (m *EdgeMetadata) Merge(other EdgeMetadata) { } } -// Flatten sums two EdgeMetadatas, their 'Window's should be the same size. The -// two EdgeMetadatas should represent different edges at the same time. +// Flatten sums two EdgeMetadatas. Their windows should be the same duration; +// they should represent different edges at the same time. func (m *EdgeMetadata) Flatten(other EdgeMetadata) { if other.WithBytes { m.WithBytes = true diff --git a/report/merge_test.go b/report/merge_test.go index 78007a59b8..80f2d0b153 100644 --- a/report/merge_test.go +++ b/report/merge_test.go @@ -1,69 +1,71 @@ -package report +package report_test import ( "reflect" "testing" + + "github.com/weaveworks/scope/report" ) func TestMergeAdjacency(t *testing.T) { for name, c := range map[string]struct { - a, b, want Adjacency + a, b, want report.Adjacency }{ "Empty b": { - a: Adjacency{ - "hostA|:192.168.1.1:12345": MakeIDList(":192.168.1.2:80"), - "hostA|:192.168.1.1:8888": MakeIDList(":1.2.3.4:22"), - "hostB|:192.168.1.2:80": MakeIDList(":192.168.1.1:12345"), + a: report.Adjacency{ + "hostA|:192.168.1.1:12345": report.MakeIDList(":192.168.1.2:80"), + "hostA|:192.168.1.1:8888": report.MakeIDList(":1.2.3.4:22"), + "hostB|:192.168.1.2:80": report.MakeIDList(":192.168.1.1:12345"), }, - b: Adjacency{}, - want: Adjacency{ - "hostA|:192.168.1.1:12345": MakeIDList(":192.168.1.2:80"), - "hostA|:192.168.1.1:8888": MakeIDList(":1.2.3.4:22"), - "hostB|:192.168.1.2:80": MakeIDList(":192.168.1.1:12345"), + b: report.Adjacency{}, + want: report.Adjacency{ + "hostA|:192.168.1.1:12345": report.MakeIDList(":192.168.1.2:80"), + "hostA|:192.168.1.1:8888": report.MakeIDList(":1.2.3.4:22"), + "hostB|:192.168.1.2:80": report.MakeIDList(":192.168.1.1:12345"), }, }, "Empty a": { - a: Adjacency{}, - b: Adjacency{ - "hostA|:192.168.1.1:12345": MakeIDList(":192.168.1.2:80"), - "hostA|:192.168.1.1:8888": MakeIDList(":1.2.3.4:22"), - "hostB|:192.168.1.2:80": MakeIDList(":192.168.1.1:12345"), + a: report.Adjacency{}, + b: report.Adjacency{ + "hostA|:192.168.1.1:12345": report.MakeIDList(":192.168.1.2:80"), + "hostA|:192.168.1.1:8888": report.MakeIDList(":1.2.3.4:22"), + "hostB|:192.168.1.2:80": report.MakeIDList(":192.168.1.1:12345"), }, - want: Adjacency{ - "hostA|:192.168.1.1:12345": MakeIDList(":192.168.1.2:80"), - "hostA|:192.168.1.1:8888": MakeIDList(":1.2.3.4:22"), - "hostB|:192.168.1.2:80": MakeIDList(":192.168.1.1:12345"), + want: report.Adjacency{ + "hostA|:192.168.1.1:12345": report.MakeIDList(":192.168.1.2:80"), + "hostA|:192.168.1.1:8888": report.MakeIDList(":1.2.3.4:22"), + "hostB|:192.168.1.2:80": report.MakeIDList(":192.168.1.1:12345"), }, }, "Same address": { - a: Adjacency{ - "hostA|:192.168.1.1:12345": MakeIDList(":192.168.1.2:80"), + a: report.Adjacency{ + "hostA|:192.168.1.1:12345": report.MakeIDList(":192.168.1.2:80"), }, - b: Adjacency{ - "hostA|:192.168.1.1:12345": MakeIDList(":192.168.1.2:8080"), + b: report.Adjacency{ + "hostA|:192.168.1.1:12345": report.MakeIDList(":192.168.1.2:8080"), }, - want: Adjacency{ - "hostA|:192.168.1.1:12345": MakeIDList( + want: report.Adjacency{ + "hostA|:192.168.1.1:12345": report.MakeIDList( ":192.168.1.2:80", ":192.168.1.2:8080", ), }, }, "No duplicates": { - a: Adjacency{ - "hostA|:192.168.1.1:12345": MakeIDList( + a: report.Adjacency{ + "hostA|:192.168.1.1:12345": report.MakeIDList( ":192.168.1.2:80", ":192.168.1.2:8080", ":192.168.1.2:555", ), }, - b: Adjacency{ - "hostA|:192.168.1.1:12345": MakeIDList( + b: report.Adjacency{ + "hostA|:192.168.1.1:12345": report.MakeIDList( ":192.168.1.2:8080", ":192.168.1.2:80", ":192.168.1.2:444", ), }, - want: Adjacency{ + want: report.Adjacency{ "hostA|:192.168.1.1:12345": []string{ ":192.168.1.2:444", ":192.168.1.2:555", @@ -73,24 +75,23 @@ func TestMergeAdjacency(t *testing.T) { }, }, "Double keys": { - a: Adjacency{ - "key1": MakeIDList("a", "c", "d", "b"), - "key2": MakeIDList("c", "a"), + a: report.Adjacency{ + "key1": report.MakeIDList("a", "c", "d", "b"), + "key2": report.MakeIDList("c", "a"), }, - b: Adjacency{ - "key1": MakeIDList("a", "b", "e"), - "key3": MakeIDList("e", "a", "a", "a", "e"), + b: report.Adjacency{ + "key1": report.MakeIDList("a", "b", "e"), + "key3": report.MakeIDList("e", "a", "a", "a", "e"), }, - want: Adjacency{ - "key1": MakeIDList("a", "b", "c", "d", "e"), - "key2": MakeIDList("a", "c"), - "key3": MakeIDList("a", "e"), + want: report.Adjacency{ + "key1": report.MakeIDList("a", "b", "c", "d", "e"), + "key2": report.MakeIDList("a", "c"), + "key3": report.MakeIDList("a", "e"), }, }, } { have := c.a have.Merge(c.b) - if !reflect.DeepEqual(c.want, have) { t.Errorf("%s: want\n\t%#v\nhave\n\t%#v", name, c.want, have) } @@ -99,12 +100,12 @@ func TestMergeAdjacency(t *testing.T) { func TestMergeEdgeMetadatas(t *testing.T) { for name, c := range map[string]struct { - a, b, want EdgeMetadatas + a, b, want report.EdgeMetadatas }{ "Empty a": { - a: EdgeMetadatas{}, - b: EdgeMetadatas{ - "hostA|:192.168.1.1:12345|:192.168.1.2:80": EdgeMetadata{ + a: report.EdgeMetadatas{}, + b: report.EdgeMetadatas{ + "hostA|:192.168.1.1:12345|:192.168.1.2:80": report.EdgeMetadata{ WithBytes: true, BytesEgress: 12, BytesIngress: 0, @@ -112,8 +113,8 @@ func TestMergeEdgeMetadatas(t *testing.T) { MaxConnCountTCP: 2, }, }, - want: EdgeMetadatas{ - "hostA|:192.168.1.1:12345|:192.168.1.2:80": EdgeMetadata{ + want: report.EdgeMetadatas{ + "hostA|:192.168.1.1:12345|:192.168.1.2:80": report.EdgeMetadata{ WithBytes: true, BytesEgress: 12, BytesIngress: 0, @@ -123,16 +124,16 @@ func TestMergeEdgeMetadatas(t *testing.T) { }, }, "Empty b": { - a: EdgeMetadatas{ - "hostA|:192.168.1.1:12345|:192.168.1.2:80": EdgeMetadata{ + a: report.EdgeMetadatas{ + "hostA|:192.168.1.1:12345|:192.168.1.2:80": report.EdgeMetadata{ WithBytes: true, BytesEgress: 12, BytesIngress: 0, }, }, - b: EdgeMetadatas{}, - want: EdgeMetadatas{ - "hostA|:192.168.1.1:12345|:192.168.1.2:80": EdgeMetadata{ + b: report.EdgeMetadatas{}, + want: report.EdgeMetadatas{ + "hostA|:192.168.1.1:12345|:192.168.1.2:80": report.EdgeMetadata{ WithBytes: true, BytesEgress: 12, BytesIngress: 0, @@ -140,8 +141,8 @@ func TestMergeEdgeMetadatas(t *testing.T) { }, }, "Host merge": { - a: EdgeMetadatas{ - "hostA|:192.168.1.1:12345|:192.168.1.2:80": EdgeMetadata{ + a: report.EdgeMetadatas{ + "hostA|:192.168.1.1:12345|:192.168.1.2:80": report.EdgeMetadata{ WithBytes: true, BytesEgress: 12, BytesIngress: 0, @@ -149,8 +150,8 @@ func TestMergeEdgeMetadatas(t *testing.T) { MaxConnCountTCP: 4, }, }, - b: EdgeMetadatas{ - "hostQ|:192.168.1.1:12345|:192.168.1.2:80": EdgeMetadata{ + b: report.EdgeMetadatas{ + "hostQ|:192.168.1.1:12345|:192.168.1.2:80": report.EdgeMetadata{ WithBytes: true, BytesEgress: 1, BytesIngress: 2, @@ -158,15 +159,15 @@ func TestMergeEdgeMetadatas(t *testing.T) { MaxConnCountTCP: 6, }, }, - want: EdgeMetadatas{ - "hostA|:192.168.1.1:12345|:192.168.1.2:80": EdgeMetadata{ + want: report.EdgeMetadatas{ + "hostA|:192.168.1.1:12345|:192.168.1.2:80": report.EdgeMetadata{ WithBytes: true, BytesEgress: 12, BytesIngress: 0, WithConnCountTCP: true, MaxConnCountTCP: 4, }, - "hostQ|:192.168.1.1:12345|:192.168.1.2:80": EdgeMetadata{ + "hostQ|:192.168.1.1:12345|:192.168.1.2:80": report.EdgeMetadata{ WithBytes: true, BytesEgress: 1, BytesIngress: 2, @@ -176,8 +177,8 @@ func TestMergeEdgeMetadatas(t *testing.T) { }, }, "Edge merge": { - a: EdgeMetadatas{ - "hostA|:192.168.1.1:12345|:192.168.1.2:80": EdgeMetadata{ + a: report.EdgeMetadatas{ + "hostA|:192.168.1.1:12345|:192.168.1.2:80": report.EdgeMetadata{ WithBytes: true, BytesEgress: 12, BytesIngress: 0, @@ -185,8 +186,8 @@ func TestMergeEdgeMetadatas(t *testing.T) { MaxConnCountTCP: 7, }, }, - b: EdgeMetadatas{ - "hostA|:192.168.1.1:12345|:192.168.1.2:80": EdgeMetadata{ + b: report.EdgeMetadatas{ + "hostA|:192.168.1.1:12345|:192.168.1.2:80": report.EdgeMetadata{ WithBytes: true, BytesEgress: 1, BytesIngress: 2, @@ -194,8 +195,8 @@ func TestMergeEdgeMetadatas(t *testing.T) { MaxConnCountTCP: 9, }, }, - want: EdgeMetadatas{ - "hostA|:192.168.1.1:12345|:192.168.1.2:80": EdgeMetadata{ + want: report.EdgeMetadatas{ + "hostA|:192.168.1.1:12345|:192.168.1.2:80": report.EdgeMetadata{ WithBytes: true, BytesEgress: 13, BytesIngress: 2, @@ -216,19 +217,19 @@ func TestMergeEdgeMetadatas(t *testing.T) { func TestMergeNodeMetadatas(t *testing.T) { for name, c := range map[string]struct { - a, b, want NodeMetadatas + a, b, want report.NodeMetadatas }{ "Empty a": { - a: NodeMetadatas{}, - b: NodeMetadatas{ - ":192.168.1.1:12345": NodeMetadata{ + a: report.NodeMetadatas{}, + b: report.NodeMetadatas{ + ":192.168.1.1:12345": report.NodeMetadata{ "pid": "23128", "name": "curl", "domain": "node-a.local", }, }, - want: NodeMetadatas{ - ":192.168.1.1:12345": NodeMetadata{ + want: report.NodeMetadatas{ + ":192.168.1.1:12345": report.NodeMetadata{ "pid": "23128", "name": "curl", "domain": "node-a.local", @@ -236,16 +237,16 @@ func TestMergeNodeMetadatas(t *testing.T) { }, }, "Empty b": { - a: NodeMetadatas{ - ":192.168.1.1:12345": NodeMetadata{ + a: report.NodeMetadatas{ + ":192.168.1.1:12345": report.NodeMetadata{ "pid": "23128", "name": "curl", "domain": "node-a.local", }, }, - b: NodeMetadatas{}, - want: NodeMetadatas{ - ":192.168.1.1:12345": NodeMetadata{ + b: report.NodeMetadatas{}, + want: report.NodeMetadatas{ + ":192.168.1.1:12345": report.NodeMetadata{ "pid": "23128", "name": "curl", "domain": "node-a.local", @@ -253,27 +254,27 @@ func TestMergeNodeMetadatas(t *testing.T) { }, }, "Simple merge": { - a: NodeMetadatas{ - ":192.168.1.1:12345": NodeMetadata{ + a: report.NodeMetadatas{ + ":192.168.1.1:12345": report.NodeMetadata{ "pid": "23128", "name": "curl", "domain": "node-a.local", }, }, - b: NodeMetadatas{ - ":192.168.1.2:12345": NodeMetadata{ + b: report.NodeMetadatas{ + ":192.168.1.2:12345": report.NodeMetadata{ "pid": "42", "name": "curl", "domain": "node-a.local", }, }, - want: NodeMetadatas{ - ":192.168.1.1:12345": NodeMetadata{ + want: report.NodeMetadatas{ + ":192.168.1.1:12345": report.NodeMetadata{ "pid": "23128", "name": "curl", "domain": "node-a.local", }, - ":192.168.1.2:12345": NodeMetadata{ + ":192.168.1.2:12345": report.NodeMetadata{ "pid": "42", "name": "curl", "domain": "node-a.local", @@ -281,22 +282,22 @@ func TestMergeNodeMetadatas(t *testing.T) { }, }, "Merge conflict": { - a: NodeMetadatas{ - ":192.168.1.1:12345": NodeMetadata{ + a: report.NodeMetadatas{ + ":192.168.1.1:12345": report.NodeMetadata{ "pid": "23128", "name": "curl", "domain": "node-a.local", }, }, - b: NodeMetadatas{ - ":192.168.1.1:12345": NodeMetadata{ // <-- same ID + b: report.NodeMetadatas{ + ":192.168.1.1:12345": report.NodeMetadata{ // <-- same ID "pid": "0", "name": "curl", "domain": "node-a.local", }, }, - want: NodeMetadatas{ - ":192.168.1.1:12345": NodeMetadata{ + want: report.NodeMetadatas{ + ":192.168.1.1:12345": report.NodeMetadata{ "pid": "23128", "name": "curl", "domain": "node-a.local", diff --git a/test/diff.go b/test/diff.go index 59ecb15c5c..d7542bb699 100644 --- a/test/diff.go +++ b/test/diff.go @@ -9,7 +9,7 @@ func init() { spew.Config.SortKeys = true // :\ } -// Diff diff diff +// Diff diffs two arbitrary data structures, giving human-readable output. func Diff(want, have interface{}) string { text, _ := difflib.GetUnifiedDiffString(difflib.UnifiedDiff{ A: difflib.SplitLines(spew.Sdump(want)), diff --git a/xfer/collector_test.go b/xfer/collector_internal_test.go similarity index 100% rename from xfer/collector_test.go rename to xfer/collector_internal_test.go