From 3797d717999063d40f7cd2cbd04ae1024c843186 Mon Sep 17 00:00:00 2001 From: Leif Madsen Date: Wed, 2 Dec 2020 15:48:04 -0500 Subject: [PATCH 1/6] Add golangci-lint action to CI --- .github/workflows/main.yml | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 93c5e586..c7260fdd 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -4,6 +4,17 @@ env: on: push jobs: + golangci: + name: Linting + runs-on: ubuntu-20.04 + steps: + - uses: actions/checkout@v2 + - name: golangci-lint + uses: golangci/golangci-lint-action@v2 + with: + # Required: the version of golangci-lint is required and must be specified without patch version: we always use the latest patch version. + version: v1.29 + test-framework: name: Base testing runs-on: ubuntu-20.04 From 79e2ff0a41950e4839d8d5036a2f568a0c2ada12 Mon Sep 17 00:00:00 2001 From: Leif Madsen Date: Wed, 2 Dec 2020 16:46:08 -0500 Subject: [PATCH 2/6] Silently drop the return codes during unit testing TODO: we will fix this during the code refactoring phase over the next sprint or so. For now let's get linting to pass so that it can check for any new issues we introduce during the refactoring. TODO items have been added. We need to review these unit tests anyways since they are currently failing. --- pkg/cacheutil/cacheserver_test.go | 5 ++++- pkg/unixserver/server.go | 6 +++++- pkg/unixserver/server_test.go | 5 ++++- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/pkg/cacheutil/cacheserver_test.go b/pkg/cacheutil/cacheserver_test.go index 4fd2a78e..5ebb81e9 100644 --- a/pkg/cacheutil/cacheserver_test.go +++ b/pkg/cacheutil/cacheserver_test.go @@ -70,7 +70,10 @@ func TestCacheExpiry(t *testing.T) { cs := NewCacheServer() ctx := context.Background() - go cs.Run(ctx) + // TODO: we should be capturing the error not ignoring it + go func() { + _ = cs.Run(ctx) + }() t.Run("single entry", func(t *testing.T) { ms.addMetric("test-metric", 1, 1, cs) diff --git a/pkg/unixserver/server.go b/pkg/unixserver/server.go index c558bca3..3ddf1805 100644 --- a/pkg/unixserver/server.go +++ b/pkg/unixserver/server.go @@ -420,7 +420,11 @@ func Listen(ctx context.Context, address string, w *bufio.Writer, registry *prom // cache server cache := cacheutil.NewCacheServer() - go cache.Run(ctx) + + // TODO: check for error returns properly + go func() { + _ = cache.Run(ctx) + }() go func() { cd := new(collectd.Collectd) diff --git a/pkg/unixserver/server_test.go b/pkg/unixserver/server_test.go index 90e790e2..6fdd28ae 100644 --- a/pkg/unixserver/server_test.go +++ b/pkg/unixserver/server_test.go @@ -29,7 +29,10 @@ func TestCDMetrics(t *testing.T) { cs.Interval = 1 ctx := context.Background() - go cs.Run(ctx) + // TODO: check for error returns properly + go func() { + _ = cs.Run(ctx) + }() cdmetrics.updateOrAddMetrics(cd, cs, 1.0) assert.Equals(t, 1, len(cdmetrics.metrics)) From 45b2a9dd7356297fa7d97771140bf3fb6f136f3b Mon Sep 17 00:00:00 2001 From: Leif Madsen Date: Wed, 2 Dec 2020 16:49:07 -0500 Subject: [PATCH 3/6] Use newest linting version --- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index c7260fdd..ce1eb622 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -13,7 +13,7 @@ jobs: uses: golangci/golangci-lint-action@v2 with: # Required: the version of golangci-lint is required and must be specified without patch version: we always use the latest patch version. - version: v1.29 + version: v1.33 test-framework: name: Base testing From b18b17142c9de53eb8af7ed7a12eca5d49cc3f27 Mon Sep 17 00:00:00 2001 From: pleimer Date: Wed, 2 Dec 2020 17:23:23 -0500 Subject: [PATCH 4/6] assert errors in tests --- pkg/cacheutil/cacheserver_test.go | 4 ++-- pkg/unixserver/server_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/cacheutil/cacheserver_test.go b/pkg/cacheutil/cacheserver_test.go index 5ebb81e9..46d1cf8e 100644 --- a/pkg/cacheutil/cacheserver_test.go +++ b/pkg/cacheutil/cacheserver_test.go @@ -70,9 +70,9 @@ func TestCacheExpiry(t *testing.T) { cs := NewCacheServer() ctx := context.Background() - // TODO: we should be capturing the error not ignoring it go func() { - _ = cs.Run(ctx) + err := cs.Run(ctx) + assert.Ok(t, err) }() t.Run("single entry", func(t *testing.T) { diff --git a/pkg/unixserver/server_test.go b/pkg/unixserver/server_test.go index 6fdd28ae..744c1f53 100644 --- a/pkg/unixserver/server_test.go +++ b/pkg/unixserver/server_test.go @@ -29,9 +29,9 @@ func TestCDMetrics(t *testing.T) { cs.Interval = 1 ctx := context.Background() - // TODO: check for error returns properly go func() { - _ = cs.Run(ctx) + err := cs.Run(ctx) + assert.Ok(t, err) }() cdmetrics.updateOrAddMetrics(cd, cs, 1.0) From 9d8cc81706c6e1515ba78767aca56451e706455a Mon Sep 17 00:00:00 2001 From: pleimer Date: Thu, 3 Dec 2020 09:49:48 -0500 Subject: [PATCH 5/6] Fix unit tests --- pkg/cacheutil/cacheserver_test.go | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/pkg/cacheutil/cacheserver_test.go b/pkg/cacheutil/cacheserver_test.go index 46d1cf8e..590f29fc 100644 --- a/pkg/cacheutil/cacheserver_test.go +++ b/pkg/cacheutil/cacheserver_test.go @@ -2,7 +2,6 @@ package cacheutil import ( "context" - "fmt" "strconv" "testing" "time" @@ -46,12 +45,12 @@ func (ms *MetricStash) addMetric(metricName string, interval float64, numLabels labelName := "test-label-" + strconv.Itoa(i) ls.deleteFn = func() { - fmt.Printf("Label %s in metric %s deleted\n", labelName, metricName) + //fmt.Printf("Label %s in metric %s deleted\n", labelName, metricName) delete(ms.metrics[metricName], labelName) if len(ms.metrics[metricName]) == 0 { delete(ms.metrics, metricName) - fmt.Printf("Metrics %s deleted\n", metricName) + //fmt.Printf("Metrics %s deleted\n", metricName) } } @@ -68,6 +67,7 @@ func TestCacheExpiry(t *testing.T) { ms := NewMetricStash() cs := NewCacheServer() + cs.Interval = 1 ctx := context.Background() go func() { @@ -78,10 +78,7 @@ func TestCacheExpiry(t *testing.T) { t.Run("single entry", func(t *testing.T) { ms.addMetric("test-metric", 1, 1, cs) assert.Equals(t, 1, len(ms.metrics)) - for i := 0; i < 2; i++ { - time.Sleep(time.Second * 1) - } - + time.Sleep(time.Millisecond * 1200) assert.Equals(t, 0, len(ms.metrics)) }) @@ -90,9 +87,7 @@ func TestCacheExpiry(t *testing.T) { ms.addMetric("test-metric-2", 2, 1, cs) assert.Equals(t, 2, len(ms.metrics)) - for i := 0; i < 4; i++ { - time.Sleep(time.Second * 1) - } + time.Sleep(time.Millisecond * 3000) assert.Equals(t, 0, len(ms.metrics)) }) @@ -100,9 +95,7 @@ func TestCacheExpiry(t *testing.T) { ms.addMetric("test-metric-1", 1, 10, cs) assert.Equals(t, 10, len(ms.metrics["test-metric-1"])) - for i := 0; i < 2; i++ { - time.Sleep(time.Second * 1) - } + time.Sleep(time.Millisecond * 2000) assert.Equals(t, 0, len(ms.metrics)) }) } From 6ee6dd51718d6078764b7d93726235ff82741a73 Mon Sep 17 00:00:00 2001 From: Leif Madsen Date: Thu, 3 Dec 2020 10:25:22 -0500 Subject: [PATCH 6/6] Re-enable go test in CI --- build/test-framework/run_tests.sh | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/build/test-framework/run_tests.sh b/build/test-framework/run_tests.sh index 6ce3eb8c..fcc76ad4 100755 --- a/build/test-framework/run_tests.sh +++ b/build/test-framework/run_tests.sh @@ -16,13 +16,11 @@ go get -u honnef.co/go/tools/cmd/staticcheck # run code validation tools echo " *** Running pre-commit code validation" -echo " --- [TODO] Tests expected to fail currently. Changes required to pass all testing. Disable for now." ./build/test-framework/pre-commit # run unit tests echo " *** Running test suite" -echo " --- [TODO] Re-enable the test suite once supporting changes result in tests to pass." -#go test -v ./... +go test -v ./... set +e echo " *** Running code coverage tooling"