From 90f9fe584c5b93aaed6c52c1fc67d60dd01119a4 Mon Sep 17 00:00:00 2001 From: "dan.markhasin" Date: Mon, 7 Jun 2021 23:05:43 +0300 Subject: [PATCH 1/5] set default execution period to avoid panic --- options.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/options.go b/options.go index 9014cd2..815c980 100644 --- a/options.go +++ b/options.go @@ -37,7 +37,11 @@ func WithHealthListeners(listener ...HealthListener) HealthOption { // WithDefaults sets all the Health object settings. It's not required to use this as no options is always default // This is a simple placeholder for any future defaults func WithDefaults() HealthOption { - return healthOptionFunc(func(h *health) {}) + return healthOptionFunc(func(h *health) { + if h.defaultExecutionPeriod == 0 { + h.defaultExecutionPeriod = 1 * time.Minute + } + }) } // CheckOption configures a health check using the functional options paradigm From 3e941744d07e333b206607d2ee5b04928b7c22d0 Mon Sep 17 00:00:00 2001 From: "dan.markhasin" Date: Tue, 8 Jun 2021 07:54:14 +0300 Subject: [PATCH 2/5] set default execution period to avoid panic --- options.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/options.go b/options.go index 815c980..85888ae 100644 --- a/options.go +++ b/options.go @@ -38,7 +38,7 @@ func WithHealthListeners(listener ...HealthListener) HealthOption { // This is a simple placeholder for any future defaults func WithDefaults() HealthOption { return healthOptionFunc(func(h *health) { - if h.defaultExecutionPeriod == 0 { + if h.defaultExecutionPeriod <= 0 { h.defaultExecutionPeriod = 1 * time.Minute } }) From b09a7a005821b4ca4eba563e39d366894d370147 Mon Sep 17 00:00:00 2001 From: "dan.markhasin" Date: Sun, 20 Jun 2021 10:49:58 +0300 Subject: [PATCH 3/5] return an error if execution period is unset --- health.go | 11 +++++++++-- health_test.go | 15 +++++++++++++++ options.go | 6 +----- 3 files changed, 25 insertions(+), 7 deletions(-) diff --git a/health.go b/health.go index 9d68d4c..da41f64 100644 --- a/health.go +++ b/health.go @@ -59,12 +59,19 @@ type health struct { } func (h *health) RegisterCheck(check Check, opts ...CheckOption) error { - if check == nil || check.Name() == "" { - return errors.Errorf("misconfigured check %v", check) + if check == nil { + return errors.New("check must not be nil") + } + if check.Name() == "" { + return errors.New("check name must not be empty") } cfg := h.initCheckConfig(opts) + if cfg.executionPeriod <= 0 { + return errors.New("execution period must be greater than 0") + } + // checks are initially failing by default, but we allow overrides... var initialErr error if !cfg.initiallyPassing { diff --git a/health_test.go b/health_test.go index 9a6851a..36a0992 100644 --- a/health_test.go +++ b/health_test.go @@ -52,6 +52,21 @@ func TestHealthWithBogusCheck(t *testing.T) { assert.Empty(t, results, "results after bogus register") } +func TestRegisterCheckValidations(t *testing.T) { + h := New() + + // should return an error for nil check + assert.EqualError(t, h.RegisterCheck(nil), "check must not be nil") + // should return an error for missing check name + assert.EqualError(t, h.RegisterCheck(&checks.CustomCheck{}), "check name must not be empty") + // Should return an error for missing execution period + assert.EqualError(t, h.RegisterCheck(&checks.CustomCheck{CheckName: "non-empty"}), "execution period must be greater than 0") + + hWithExecPeriod := New(ExecutionPeriod(1 * time.Minute)) + assert.NoError(t, hWithExecPeriod.RegisterCheck(&checks.CustomCheck{CheckName: "non-empty"})) + +} + func TestRegisterDeregister(t *testing.T) { leaktest.Check(t) diff --git a/options.go b/options.go index 85888ae..9014cd2 100644 --- a/options.go +++ b/options.go @@ -37,11 +37,7 @@ func WithHealthListeners(listener ...HealthListener) HealthOption { // WithDefaults sets all the Health object settings. It's not required to use this as no options is always default // This is a simple placeholder for any future defaults func WithDefaults() HealthOption { - return healthOptionFunc(func(h *health) { - if h.defaultExecutionPeriod <= 0 { - h.defaultExecutionPeriod = 1 * time.Minute - } - }) + return healthOptionFunc(func(h *health) {}) } // CheckOption configures a health check using the functional options paradigm From d9b6a7714036eb2872f2f8a24d1a5c428dde3325 Mon Sep 17 00:00:00 2001 From: "dan.markhasin" Date: Sun, 20 Jun 2021 10:54:39 +0300 Subject: [PATCH 4/5] fixed test --- health_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/health_test.go b/health_test.go index 36a0992..1c51667 100644 --- a/health_test.go +++ b/health_test.go @@ -43,8 +43,7 @@ func TestHealthWithBogusCheck(t *testing.T) { err := h.RegisterCheck(nil) defer h.DeregisterAll() - assert.Error(t, err, "register bogus check should fail") - assert.Contains(t, err.Error(), "misconfigured check", "fail message") + assert.EqualError(t, err, "check must not be nil") assert.True(t, h.IsHealthy(), "health after bogus register") results, healthy := h.Results() @@ -54,6 +53,7 @@ func TestHealthWithBogusCheck(t *testing.T) { func TestRegisterCheckValidations(t *testing.T) { h := New() + defer h.DeregisterAll() // should return an error for nil check assert.EqualError(t, h.RegisterCheck(nil), "check must not be nil") @@ -63,6 +63,8 @@ func TestRegisterCheckValidations(t *testing.T) { assert.EqualError(t, h.RegisterCheck(&checks.CustomCheck{CheckName: "non-empty"}), "execution period must be greater than 0") hWithExecPeriod := New(ExecutionPeriod(1 * time.Minute)) + defer hWithExecPeriod.DeregisterAll() + assert.NoError(t, hWithExecPeriod.RegisterCheck(&checks.CustomCheck{CheckName: "non-empty"})) } From 5a33dc670e0cd03f9e34aa9bfe48f95d12189a1a Mon Sep 17 00:00:00 2001 From: "dan.markhasin" Date: Sun, 20 Jun 2021 10:58:01 +0300 Subject: [PATCH 5/5] comment --- health_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/health_test.go b/health_test.go index 1c51667..3c42f50 100644 --- a/health_test.go +++ b/health_test.go @@ -65,6 +65,7 @@ func TestRegisterCheckValidations(t *testing.T) { hWithExecPeriod := New(ExecutionPeriod(1 * time.Minute)) defer hWithExecPeriod.DeregisterAll() + // should inherit the execution period from the health instance assert.NoError(t, hWithExecPeriod.RegisterCheck(&checks.CustomCheck{CheckName: "non-empty"})) }