[OSC-829] Refactoring unit test #1631

harshshahsumo · 2024-07-22T20:44:26Z

Refactoring unit test. Link to the ticket https://sumologic.atlassian.net/browse/OSC-829

rnishtala-sumo · 2024-07-22T20:56:48Z

pkg/extension/opampextension/opamp_agent_test.go

@@ -31,12 +31,36 @@ import (
 	semconv "go.opentelemetry.io/collector/semconv/v1.18.0"
 )

-func TestNewOpampAgent(t *testing.T) {
-	cfg := createDefaultConfig()
+func setupOpampAgent(t *testing.T) (*opampAgent, *Config) {


I believe the goal here is to convert this to be parameterized tests in go - https://stackoverflow.com/questions/72856565/go-golang-test-parameterized

Yes, we should follow the manner:

sumologic-otel-collector/pkg/exporter/sumologicexporter/config_test.go

Lines 12 to 111 in c7417c6

func TestInitExporterInvalidConfiguration(t *testing.T) {

testcases := []struct {

name string

cfg *Config

expectedError error

}{

{

name: "unexpected log format",

expectedError: errors.New("unexpected log format: test_format"),

cfg: &Config{

LogFormat: "test_format",

MetricFormat: "otlp",

TraceFormat: "otlp",

ClientConfig: confighttp.ClientConfig{

Timeout: defaultTimeout,

Endpoint: "test_endpoint",

},

},

},

{

name: "unexpected metric format",

expectedError: errors.New("unexpected metric format: test_format"),

cfg: &Config{

LogFormat: "json",

MetricFormat: "test_format",

ClientConfig: confighttp.ClientConfig{

Timeout: defaultTimeout,

Endpoint: "test_endpoint",

Compression: "gzip",

},

},

},

{

name: "unsupported Carbon2 metrics format",

expectedError: errors.New("support for the carbon2 metric format was removed, please use prometheus or otlp instead"),

cfg: &Config{

LogFormat: "json",

MetricFormat: "carbon2",

ClientConfig: confighttp.ClientConfig{

Timeout: defaultTimeout,

Endpoint: "test_endpoint",

Compression: "gzip",

},

},

},

{

name: "unsupported Graphite metrics format",

expectedError: errors.New("support for the graphite metric format was removed, please use prometheus or otlp instead"),

cfg: &Config{

LogFormat: "json",

MetricFormat: "graphite",

ClientConfig: confighttp.ClientConfig{

Timeout: defaultTimeout,

Endpoint: "test_endpoint",

Compression: "gzip",

},

},

},

{

name: "unexpected trace format",

expectedError: errors.New("unexpected trace format: text"),

cfg: &Config{

LogFormat: "json",

MetricFormat: "otlp",

TraceFormat: "text",

ClientConfig: confighttp.ClientConfig{

Timeout: defaultTimeout,

Endpoint: "test_endpoint",

Compression: "gzip",

},

},

},

{

name: "no endpoint and no auth extension specified",

expectedError: errors.New("no endpoint and no auth extension specified"),

cfg: &Config{

LogFormat: "json",

MetricFormat: "otlp",

TraceFormat: "otlp",

ClientConfig: confighttp.ClientConfig{

Timeout: defaultTimeout,

Compression: "gzip",

},

},

},

}

for _, tc := range testcases {

tc := tc

t.Run(tc.name, func(t *testing.T) {

err := component.ValidateConfig(tc.cfg)

if tc.expectedError != nil {

assert.EqualError(t, err, tc.expectedError.Error())

} else {

assert.NoError(t, err)

}

})

}

}

so, we have the testcases struct with parameters and we iterate over it, so instead of having 5 different Test functions, we have just 5-element list

sumo-drosiek · 2024-07-26T06:42:57Z

pkg/extension/opampextension/opamp_agent_test.go

+			setup: func() (*Config, extension.Settings) {
+				cfg := createDefaultConfig()
+				set := extensiontest.NewNopSettings()
+				set.BuildInfo = component.BuildInfo{Version: "test version", Command: "otelcoltest"}
+				return cfg.(*Config), set
+			},


I see the setup is the same or almost the same across many tests? Can we unify it, so we put it into test body and not as test argument?

If there are more groups (2) we can split them to two tests

In general we shouldn't repeat any code if thats possible

sumo-drosiek · 2024-07-26T06:52:59Z

I am have an impression that you are trying to put all tests into one function, but not sure if thats possible. The aim of the issue was to group the tests such way, we avoid copying code. In current shape, we still copy code, just we do it in the struct instead of having separate functions

chan-tim-sumo · 2024-08-01T00:37:02Z

pkg/extension/opampextension/opamp_agent_test.go

+				rc := remoteConfig(t, path)
+
+				changed, err := o.applyRemoteConfig(rc)
+				assert.NoError(t, err)
+				assert.True(t, changed)
+				assert.NotEqual(t, len(o.effectiveConfig), 0)
+
+				o.cfg.AcceptsRemoteConfiguration = false
+				changed, err = o.applyRemoteConfig(rc)
+				assert.False(t, changed)
+				assert.Error(t, err)
+				assert.Equal(t, "OpAMP agent does not accept remote configuration", err.Error())


seems like there're more than 2 duplicates, should this be some type of function? 🤔

chan-tim-sumo · 2024-08-02T16:54:34Z

should this be broken down into multiple (2) PRs? kinda hard to see whats being changed/refactored here 🤔

chan-tim-sumo · 2024-08-02T22:00:15Z

pkg/extension/opampextension/opamp_agent_test.go

+				assert.NotEqual(t, len(o.effectiveConfig), 0)
+
+				o.cfg.AcceptsRemoteConfiguration = false
+				changed, err = o.applyRemoteConfig(rc)


looks like alot of the ApplyRemote tests are repeated. i feel like the test should look something like this (didn't include all the different config yaml files and a little more adjustments)

func TestApplyRemoteConfig(t *testing.T) { tests := []struct { name string file string acceptsRemote bool expectChanged bool expectError bool errorMessage string }{ {"Valid Config", "testdata/opamp.d/opamp-remote-config.yaml", true, true, false, ""}, {"Apache Config", "testdata/opamp.d/opamp-apache-config.yaml", false, false, true, "OpAMP agent does not accept remote configuration"}, {"Invalid Config", "testdata/opamp.d/opamp-invalid-remote-config.yaml", true, false, true, "'max_elapsed_time' must be non-negative"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { d, err := os.MkdirTemp("", "opamp.d") assert.NoError(t, err) defer os.RemoveAll(d) cfg := createDefaultConfig().(*Config) cfg.RemoteConfigurationDirectory = d set := extensiontest.NewNopSettings() o, err := newOpampAgent(cfg, set.Logger, set.BuildInfo, set.Resource) assert.NoError(t, err) cfg.AcceptsRemoteConfiguration = tt.acceptsRemote path := filepath.Join(tt.file) rb, err := os.ReadFile(path) assert.NoError(t, err) rc := &protobufs.AgentRemoteConfig{ Config: &protobufs.AgentConfigMap{ ConfigMap: map[string]*protobufs.AgentConfigFile{ "default": { Body: rb, }, }, }, ConfigHash: []byte("b2b1e3e7f45d564db1c0b621bbf67008"), } changed, err := o.applyRemoteConfig(rc) if tt.expectError { assert.Error(t, err) assert.ErrorContains(t, err, tt.errorMessage) } else { assert.NoError(t, err) } assert.Equal(t, tt.expectChanged, changed) }) } }

so now the code isn't as repeated :D

Nice!! Thanks, updated

chan-tim-sumo · 2024-08-05T18:22:17Z

pkg/extension/opampextension/opamp_agent_test.go

+	}{
+		{
+			name: "GetAgentCapabilities",
+			validate: func(o *opampAgent, t *testing.T) {


we shouldn't have validate as a input (I see the rest of the changes have this).

as discussed offline, we reduced 15-20 lines by using defaultSetup for all the testcases in TestDefaultOpampAgent hence not touching it

I mentioned the defaultSetup was good for refactoring. I feel these tests doesn't need to be inside a table test since most of them (if any) in here doesn't have repeated code. In my opinion, validate shouldn't be an input, but in this specific test case, it isn't WRONG to leave it like this.

What does the team think about this?

I tend to agree, validations/assertions are typically not seen as a test input.

chan-tim-sumo · 2024-08-07T21:13:30Z

pkg/extension/opampextension/opamp_agent_test.go

+			cfg := createDefaultConfig().(*Config)
+			cfg.RemoteConfigurationDirectory = d
+			set := extensiontest.NewNopSettings()
+			o, err := newOpampAgent(cfg, set.Logger, set.BuildInfo, set.Resource)


might be super nit, but 2 small things:

this line newOpampAgent seems to be repeated in a bunch of tests, should this be refactored as well?

i see that this test uses createDefaultConfig(), a bunch of other tests also used that before but is now replaced with defaultSetup(), should this also use that? just want to keep things as consistent if possible

newOpampAgent is repeated in about 5+ tests

chan-tim-sumo · 2024-08-12T18:24:49Z

pkg/extension/opampextension/opamp_agent_test.go

+			cfg := createDefaultConfig().(*Config)
+			cfg.RemoteConfigurationDirectory = d
+			set := extensiontest.NewNopSettings()
+			o, err := newOpampAgent(cfg, set.Logger, set.BuildInfo, set.Resource)


newOpampAgent is repeated in about 5+ tests

chan-tim-sumo · 2024-08-12T18:25:57Z

pkg/extension/opampextension/opamp_agent_test.go

 	semconv "go.opentelemetry.io/collector/semconv/v1.18.0"
 )

-func TestNewOpampAgent(t *testing.T) {
-	cfg := createDefaultConfig()
+const errMsgRemoteConfigNotAccepted = "OpAMP agent does not accept remote configuration"


more than 1 const should in a block.

chan-tim-sumo · 2024-08-12T18:26:31Z

pkg/extension/opampextension/opamp_agent_test.go

@@ -565,14 +231,13 @@ func TestHackSetEndpoint(t *testing.T) {
 			name:         "prod sumologic url",
 			url:          "https://open-collectors.sumologic.com/api/v1",
 			wantEndpoint: "wss://opamp-collectors.sumologic.com/v1/opamp",
-		},
-		{
+		}, {


chan-tim-sumo · 2024-08-12T18:46:45Z

pkg/extension/opampextension/opamp_agent_test.go

+	return cfg, set
+}
+
+func setupWithBuildInfo(version, command string) (*Config, extension.Settings) {


can't this line be in defaultSetup?

this line was being used in the test setupWithBuildInfo("test version", "otelcoltest"), shouldn't this be in the setup function?

some functions use set.BuildInfo = component.BuildInfo{Version: version, Command: command}
while some do not, so in such places we use either defaultSetup or setupWithBuilderInfo depending upon the testcase

i agree with that, then you can use a blank identifier to make it cleaner. so that way instead of writing smaller functions, it can all be in one function instead.

[OSC-829] Refactoring unit test

010e738

harshshahsumo requested a review from a team as a code owner July 22, 2024 20:44

rnishtala-sumo reviewed Jul 22, 2024

View reviewed changes

paremetrized test

8f2d6f3

harshshahsumo added the skip-changelog label Jul 25, 2024

Merge branch 'main' into osc829_refactor_unitTest

bfcf8a4

sumo-drosiek reviewed Jul 26, 2024

View reviewed changes

chan-tim-sumo reviewed Aug 1, 2024

View reviewed changes

harshshahsumo added 3 commits August 1, 2024 16:57

Modified to cleaner code

ba3c9fc

Fixing jenkins multi execution issue

b3c069a

fmt

2a38480

Comment

be16462

chan-tim-sumo reviewed Aug 2, 2024

View reviewed changes

harshshahsumo and others added 3 commits August 5, 2024 13:14

Remodified

767861a

format

23949bf

Merge branch 'main' into osc829_refactor_unitTest

8b1a57a

chan-tim-sumo reviewed Aug 5, 2024

View reviewed changes

harshshahsumo added 5 commits August 5, 2024 15:51

slight changes

831640d

Pushing final changes

15e7979

format

017035b

reverting few testcases which are not required to be parametrized

18b3a92

lint

68acc52

chan-tim-sumo reviewed Aug 7, 2024

View reviewed changes

Addressing refactor comments

ad663a7

chan-tim-sumo reviewed Aug 12, 2024

View reviewed changes

Nit comments addressing

e501ce3

chan-tim-sumo approved these changes Aug 12, 2024

View reviewed changes

harshshahsumo added 2 commits August 12, 2024 16:10

Merge branch 'main' into osc829_refactor_unitTest

f4d2973

Merge branch 'main' into osc829_refactor_unitTest

ad06d31

harshshahsumo merged commit a4e4ab8 into main Aug 12, 2024
32 checks passed

harshshahsumo deleted the osc829_refactor_unitTest branch August 12, 2024 22:25

rnishtala-sumo mentioned this pull request Aug 15, 2024

opampdreceveiver: refactor unit tests #1616

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OSC-829] Refactoring unit test #1631

[OSC-829] Refactoring unit test #1631

harshshahsumo commented Jul 22, 2024

rnishtala-sumo Jul 22, 2024

sumo-drosiek Jul 23, 2024

harshshahsumo Aug 8, 2024

sumo-drosiek Jul 26, 2024 •

edited

Loading

harshshahsumo Aug 8, 2024

sumo-drosiek commented Jul 26, 2024

chan-tim-sumo Aug 1, 2024

chan-tim-sumo commented Aug 2, 2024

chan-tim-sumo Aug 2, 2024 •

edited

Loading

harshshahsumo Aug 5, 2024

chan-tim-sumo Aug 5, 2024 •

edited

Loading

harshshahsumo Aug 5, 2024

chan-tim-sumo Aug 5, 2024

rnishtala-sumo Aug 5, 2024

harshshahsumo Aug 7, 2024

chan-tim-sumo Aug 7, 2024 •

edited

Loading

harshshahsumo Aug 8, 2024

chan-tim-sumo Aug 12, 2024

harshshahsumo Aug 12, 2024

chan-tim-sumo Aug 12, 2024

chan-tim-sumo Aug 12, 2024

harshshahsumo Aug 12, 2024

chan-tim-sumo Aug 12, 2024

harshshahsumo Aug 12, 2024

chan-tim-sumo Aug 12, 2024 •

edited

Loading

chan-tim-sumo Aug 12, 2024 •

edited

Loading

harshshahsumo Aug 12, 2024

chan-tim-sumo Aug 12, 2024

	func TestInitExporterInvalidConfiguration(t *testing.T) {
	testcases := []struct {
	name string
	cfg *Config
	expectedError error
	}{
	{
	name: "unexpected log format",
	expectedError: errors.New("unexpected log format: test_format"),
	cfg: &Config{
	LogFormat: "test_format",
	MetricFormat: "otlp",
	TraceFormat: "otlp",
	ClientConfig: confighttp.ClientConfig{
	Timeout: defaultTimeout,
	Endpoint: "test_endpoint",
	},
	},
	},
	{
	name: "unexpected metric format",
	expectedError: errors.New("unexpected metric format: test_format"),
	cfg: &Config{
	LogFormat: "json",
	MetricFormat: "test_format",
	ClientConfig: confighttp.ClientConfig{
	Timeout: defaultTimeout,
	Endpoint: "test_endpoint",
	Compression: "gzip",
	},
	},
	},
	{
	name: "unsupported Carbon2 metrics format",
	expectedError: errors.New("support for the carbon2 metric format was removed, please use prometheus or otlp instead"),
	cfg: &Config{
	LogFormat: "json",
	MetricFormat: "carbon2",
	ClientConfig: confighttp.ClientConfig{
	Timeout: defaultTimeout,
	Endpoint: "test_endpoint",
	Compression: "gzip",
	},
	},
	},
	{
	name: "unsupported Graphite metrics format",
	expectedError: errors.New("support for the graphite metric format was removed, please use prometheus or otlp instead"),
	cfg: &Config{
	LogFormat: "json",
	MetricFormat: "graphite",
	ClientConfig: confighttp.ClientConfig{
	Timeout: defaultTimeout,
	Endpoint: "test_endpoint",
	Compression: "gzip",
	},
	},
	},
	{
	name: "unexpected trace format",
	expectedError: errors.New("unexpected trace format: text"),
	cfg: &Config{
	LogFormat: "json",
	MetricFormat: "otlp",
	TraceFormat: "text",
	ClientConfig: confighttp.ClientConfig{
	Timeout: defaultTimeout,
	Endpoint: "test_endpoint",
	Compression: "gzip",
	},
	},
	},
	{
	name: "no endpoint and no auth extension specified",
	expectedError: errors.New("no endpoint and no auth extension specified"),
	cfg: &Config{
	LogFormat: "json",
	MetricFormat: "otlp",
	TraceFormat: "otlp",
	ClientConfig: confighttp.ClientConfig{
	Timeout: defaultTimeout,
	Compression: "gzip",
	},
	},
	},
	}

	for _, tc := range testcases {
	tc := tc
	t.Run(tc.name, func(t *testing.T) {
	err := component.ValidateConfig(tc.cfg)

	if tc.expectedError != nil {
	assert.EqualError(t, err, tc.expectedError.Error())
	} else {
	assert.NoError(t, err)
	}
	})
	}
	}

[OSC-829] Refactoring unit test #1631

[OSC-829] Refactoring unit test #1631

Conversation

harshshahsumo commented Jul 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sumo-drosiek Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sumo-drosiek commented Jul 26, 2024

Choose a reason for hiding this comment

chan-tim-sumo commented Aug 2, 2024

chan-tim-sumo Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chan-tim-sumo Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chan-tim-sumo Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chan-tim-sumo Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

chan-tim-sumo Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sumo-drosiek Jul 26, 2024 •

edited

Loading

chan-tim-sumo Aug 2, 2024 •

edited

Loading

chan-tim-sumo Aug 5, 2024 •

edited

Loading

chan-tim-sumo Aug 7, 2024 •

edited

Loading

chan-tim-sumo Aug 12, 2024 •

edited

Loading

chan-tim-sumo Aug 12, 2024 •

edited

Loading