From eba4426b990ab83558da9e1754b1ce1a67218455 Mon Sep 17 00:00:00 2001 From: Neil Xie Date: Fri, 29 Dec 2023 09:03:20 -0800 Subject: [PATCH 1/2] Add unit test for new Kafka client --- .../kafka/{clientImpl.go => client_impl.go} | 0 common/messaging/kafka/client_impl_test.go | 159 ++++++++++++++++++ 2 files changed, 159 insertions(+) rename common/messaging/kafka/{clientImpl.go => client_impl.go} (100%) create mode 100644 common/messaging/kafka/client_impl_test.go diff --git a/common/messaging/kafka/clientImpl.go b/common/messaging/kafka/client_impl.go similarity index 100% rename from common/messaging/kafka/clientImpl.go rename to common/messaging/kafka/client_impl.go diff --git a/common/messaging/kafka/client_impl_test.go b/common/messaging/kafka/client_impl_test.go new file mode 100644 index 00000000000..464a6e7ae1e --- /dev/null +++ b/common/messaging/kafka/client_impl_test.go @@ -0,0 +1,159 @@ +// The MIT License (MIT) + +// Copyright (c) 2017-2020 Uber Technologies Inc. + +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +package kafka + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/uber-go/tally" + + "github.com/uber/cadence/common/config" + "github.com/uber/cadence/common/log/testlogger" + "github.com/uber/cadence/common/metrics" +) + +func TestNewKafkaClient(t *testing.T) { + metricsClient := metrics.NewClient(tally.NoopScope, metrics.History) + logger := testlogger.New(t) + testCases := []struct { + name string + config *config.KafkaConfig + checkApp bool + hasErr bool + expectedErr string + }{ + { + name: "Missing clusters", + config: &config.KafkaConfig{ + Clusters: map[string]config.ClusterConfig{}, + }, + checkApp: true, + hasErr: true, + expectedErr: "Empty Kafka Cluster Config", + }, + { + name: "Missing topics", + config: &config.KafkaConfig{ + Clusters: map[string]config.ClusterConfig{ + "testCluster": { + Brokers: []string{"testBrokers"}, + }, + }, + Topics: map[string]config.TopicConfig{}, + }, + checkApp: true, + hasErr: true, + expectedErr: "Empty Topics Config", + }, + { + name: "Missing Applications", + config: &config.KafkaConfig{ + Clusters: map[string]config.ClusterConfig{ + "test-cluster": { + Brokers: []string{"test-brokers"}, + }, + }, + Topics: map[string]config.TopicConfig{ + "test-topic": { + Cluster: "test-cluster", + }, + }, + Applications: map[string]config.TopicList{}, + }, + checkApp: true, + hasErr: true, + expectedErr: "Empty Applications Config", + }, + { + name: "Missing topics config", + config: &config.KafkaConfig{ + Clusters: map[string]config.ClusterConfig{ + "test-cluster": { + Brokers: []string{"test-brokers"}, + }, + }, + Topics: map[string]config.TopicConfig{ + "test-topic": { + Cluster: "test-cluster", + }, + }, + Applications: map[string]config.TopicList{ + "test-app": { + Topic: "test-topic", + DLQTopic: "test-topic-dlq", + }, + }, + }, + checkApp: true, + hasErr: true, + expectedErr: "Missing Topic Config for Topic test-topic-dlq", + }, + { + name: "Normal Case", + config: &config.KafkaConfig{ + Clusters: map[string]config.ClusterConfig{ + "test-cluster": { + Brokers: []string{"test-brokers"}, + }, + }, + Topics: map[string]config.TopicConfig{ + "test-topic": { + Cluster: "test-cluster", + }, + "test-topic-dlq": { + Cluster: "test-cluster", + }, + }, + Applications: map[string]config.TopicList{ + "test-app": { + Topic: "test-topic", + DLQTopic: "test-topic-dlq", + }, + }, + }, + checkApp: true, + hasErr: false, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if !tc.hasErr { + kafkaClient := NewKafkaClient(tc.config, metricsClient, logger, nil, tc.checkApp) + // Type assert to *clientImpl to access struct fields + client, ok := kafkaClient.(*clientImpl) + assert.True(t, ok, "Expected kafkaClient to be of type *clientImpl") + assert.Equal(t, tc.config, client.config) + } else { + defer func() { + if r := recover(); r == nil { + t.Errorf("The code did not panic") + } else { + assert.Equal(t, tc.expectedErr, r) + } + }() + NewKafkaClient(tc.config, metricsClient, logger, nil, tc.checkApp) + } + }) + } +} From 12a798db71ab3b7d51decf2659b45cd8dfbb4821 Mon Sep 17 00:00:00 2001 From: Neil Xie Date: Fri, 29 Dec 2023 09:41:33 -0800 Subject: [PATCH 2/2] Remove hasErr --- common/messaging/kafka/client_impl_test.go | 32 +++++++--------------- 1 file changed, 10 insertions(+), 22 deletions(-) diff --git a/common/messaging/kafka/client_impl_test.go b/common/messaging/kafka/client_impl_test.go index 464a6e7ae1e..167ca7c106e 100644 --- a/common/messaging/kafka/client_impl_test.go +++ b/common/messaging/kafka/client_impl_test.go @@ -40,7 +40,6 @@ func TestNewKafkaClient(t *testing.T) { name string config *config.KafkaConfig checkApp bool - hasErr bool expectedErr string }{ { @@ -49,7 +48,6 @@ func TestNewKafkaClient(t *testing.T) { Clusters: map[string]config.ClusterConfig{}, }, checkApp: true, - hasErr: true, expectedErr: "Empty Kafka Cluster Config", }, { @@ -63,7 +61,6 @@ func TestNewKafkaClient(t *testing.T) { Topics: map[string]config.TopicConfig{}, }, checkApp: true, - hasErr: true, expectedErr: "Empty Topics Config", }, { @@ -82,7 +79,6 @@ func TestNewKafkaClient(t *testing.T) { Applications: map[string]config.TopicList{}, }, checkApp: true, - hasErr: true, expectedErr: "Empty Applications Config", }, { @@ -106,7 +102,6 @@ func TestNewKafkaClient(t *testing.T) { }, }, checkApp: true, - hasErr: true, expectedErr: "Missing Topic Config for Topic test-topic-dlq", }, { @@ -133,27 +128,20 @@ func TestNewKafkaClient(t *testing.T) { }, }, checkApp: true, - hasErr: false, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - if !tc.hasErr { - kafkaClient := NewKafkaClient(tc.config, metricsClient, logger, nil, tc.checkApp) - // Type assert to *clientImpl to access struct fields - client, ok := kafkaClient.(*clientImpl) - assert.True(t, ok, "Expected kafkaClient to be of type *clientImpl") - assert.Equal(t, tc.config, client.config) - } else { - defer func() { - if r := recover(); r == nil { - t.Errorf("The code did not panic") - } else { - assert.Equal(t, tc.expectedErr, r) - } - }() - NewKafkaClient(tc.config, metricsClient, logger, nil, tc.checkApp) - } + defer func() { + if r := recover(); r != nil { + assert.Equal(t, tc.expectedErr, r) + } + }() + kafkaClient := NewKafkaClient(tc.config, metricsClient, logger, nil, tc.checkApp) + // Type assert to *clientImpl to access struct fields + client, ok := kafkaClient.(*clientImpl) + assert.True(t, ok, "Expected kafkaClient to be of type *clientImpl") + assert.Equal(t, tc.config, client.config) }) } }