From 21e0683e30f855d54fca2bbb0e5c8e3ff7783afb Mon Sep 17 00:00:00 2001 From: hrishikesh Date: Wed, 11 Dec 2019 05:54:46 +0530 Subject: [PATCH] Fixes broken metrics unit tests Quite often unit test failure showing "*** a different view with the same name is already registered" messages at many places. At the biggning of every metrics test, it tries to register respective metrics views(metrics data it want present), publish metrics and uniregister views. Uniregister view is important becuase for next test it dont need to hold the old metrics data. unregister() views were getting called using `defer` in table test for loop, just like for _, td := range tests { defer unregister() ....... } This were resulting unwanted execution behaviour for unregister() views function, which umtimately faling tests. Its better to unregister() views before start executing the tests. This patch fixes the behaviour by calling uniregister() views function at the end of loop. Fixes #1659 --- pkg/reconciler/pipelinerun/metrics_test.go | 7 +++++-- pkg/reconciler/taskrun/metrics_test.go | 12 +++++++----- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/pkg/reconciler/pipelinerun/metrics_test.go b/pkg/reconciler/pipelinerun/metrics_test.go index 73338c061ef..37050865395 100644 --- a/pkg/reconciler/pipelinerun/metrics_test.go +++ b/pkg/reconciler/pipelinerun/metrics_test.go @@ -93,7 +93,7 @@ func TestRecordPipelineRunDurationCount(t *testing.T) { for _, test := range testData { t.Run(test.name, func(t *testing.T) { - defer unregisterMetrics() + unregisterMetrics() metrics, err := NewRecorder() assertErrIsNil(err, "Recorder initialization failed", t) @@ -102,12 +102,13 @@ func TestRecordPipelineRunDurationCount(t *testing.T) { assertErrIsNil(err, "DurationAndCount recording recording got an error", t) metricstest.CheckDistributionData(t, "pipelinerun_duration_seconds", test.expectedTags, 1, test.expectedDuration, test.expectedDuration) metricstest.CheckCountData(t, "pipelinerun_count", test.expectedTags, test.expectedCount) + }) } } func TestRecordRunningPipelineRunsCount(t *testing.T) { - defer unregisterMetrics() + unregisterMetrics() ctx, _ := rtesting.SetupFakeContext(t) informer := fakepipelineruninformer.Get(ctx) @@ -121,6 +122,7 @@ func TestRecordRunningPipelineRunsCount(t *testing.T) { err = metrics.RunningPipelineRuns(informer.Lister()) assertErrIsNil(err, "RunningPrsCount recording expected to return nil but got error", t) metricstest.CheckLastValueData(t, "running_pipelineruns_count", map[string]string{}, 1) + } func addPipelineRun(informer alpha1.PipelineRunInformer, run, pipeline, ns string, status corev1.ConditionStatus, t *testing.T) { @@ -156,4 +158,5 @@ func assertErrIsNil(err error, message string, t *testing.T) { func unregisterMetrics() { metricstest.Unregister("pipelinerun_duration_seconds", "pipelinerun_count", "running_pipelineruns_count") + } diff --git a/pkg/reconciler/taskrun/metrics_test.go b/pkg/reconciler/taskrun/metrics_test.go index de4e9e53777..4e00a90eaf1 100644 --- a/pkg/reconciler/taskrun/metrics_test.go +++ b/pkg/reconciler/taskrun/metrics_test.go @@ -101,7 +101,7 @@ func TestRecordTaskrunDurationCount(t *testing.T) { for _, test := range testData { t.Run(test.name, func(t *testing.T) { - defer unregisterMetrics() + unregisterMetrics() metrics, err := NewRecorder() assertErrIsNil(err, "Recorder initialization failed", t) @@ -110,6 +110,7 @@ func TestRecordTaskrunDurationCount(t *testing.T) { assertErrIsNil(err, "DurationAndCount recording got an error", t) metricstest.CheckDistributionData(t, "taskrun_duration_seconds", test.expectedTags, 1, test.expectedDuration, test.expectedDuration) metricstest.CheckCountData(t, "taskrun_count", test.expectedTags, test.expectedCount) + }) } } @@ -179,8 +180,7 @@ func TestRecordPipelinerunTaskrunDurationCount(t *testing.T) { for _, test := range testData { t.Run(test.name, func(t *testing.T) { - defer unregisterMetrics() - + unregisterMetrics() metrics, err := NewRecorder() assertErrIsNil(err, "Recorder initialization failed", t) @@ -188,12 +188,13 @@ func TestRecordPipelinerunTaskrunDurationCount(t *testing.T) { assertErrIsNil(err, "DurationAndCount recording got an error", t) metricstest.CheckDistributionData(t, "pipelinerun_taskrun_duration_seconds", test.expectedTags, 1, test.expectedDuration, test.expectedDuration) metricstest.CheckCountData(t, "taskrun_count", test.expectedTags, test.expectedCount) + }) } } func TestRecordRunningTaskrunsCount(t *testing.T) { - defer unregisterMetrics() + unregisterMetrics() ctx, _ := rtesting.SetupFakeContext(t) informer := faketaskruninformer.Get(ctx) @@ -255,7 +256,7 @@ func TestRecordPodLatency(t *testing.T) { for _, td := range testData { t.Run(td.name, func(t *testing.T) { - defer unregisterMetrics() + unregisterMetrics() metrics, err := NewRecorder() assertErrIsNil(err, "Recorder initialization failed", t) @@ -267,6 +268,7 @@ func TestRecordPodLatency(t *testing.T) { } assertErrIsNil(err, "RecordPodLatency recording expected to return nil but got error", t) metricstest.CheckLastValueData(t, "taskruns_pod_latency", td.expectedTags, td.expectedValue) + }) }