Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved tests for a logging exporter and jaeger exporter #377

Merged
merged 4 commits into from
Oct 7, 2019
Merged

Improved tests for a logging exporter and jaeger exporter #377

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6c182b6
improve exporter tests for logging and jaeger
agneum Oct 3, 2019
9e9ac29
fix fmt
agneum Oct 3, 2019
af4feea
replace the assert package with the require package in exporter tests…
agneum Oct 4, 2019
18a976d
fix a test for a jaeger thrift http exporter
agneum Oct 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion exporter/jaeger/jaegergrpcexporter/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ func TestLoadConfig(t *testing.T) {
e1 := cfg.Exporters["jaeger_grpc/2"]
assert.Equal(t, "jaeger_grpc/2", e1.(*Config).Name())
assert.Equal(t, "a.new.target:1234", e1.(*Config).Endpoint)
_, err = factory.CreateTraceExporter(zap.NewNop(), e1)
te, err := factory.CreateTraceExporter(zap.NewNop(), e1)
require.NoError(t, err)
require.NotNil(t, te)
}
1 change: 1 addition & 0 deletions exporter/jaeger/jaegergrpcexporter/factory_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ func TestCreateInstanceViaFactory(t *testing.T) {
exp, err := factory.CreateTraceExporter(
zap.NewNop(),
cfg)
assert.NotNil(t, err)
assert.Equal(t, "\"jaeger_grpc\" config requires a non-empty \"endpoint\"", err.Error())
assert.Nil(t, exp)

Expand Down
3 changes: 2 additions & 1 deletion exporter/jaeger/jaegerthrifthttpexporter/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ func TestLoadConfig(t *testing.T) {
}
assert.Equal(t, &expectedCfg, e1)

_, err = factory.CreateTraceExporter(zap.NewNop(), e1)
te, err := factory.CreateTraceExporter(zap.NewNop(), e1)
require.NoError(t, err)
require.NotNil(t, te)
}
64 changes: 37 additions & 27 deletions exporter/jaeger/jaegerthrifthttpexporter/exporter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,28 +25,20 @@ import (
"github.com/open-telemetry/opentelemetry-collector/consumer/consumerdata"
)

func TestNew(t *testing.T) {
const testHTTPAddress = "http://a.test.dom:123/at/some/path"
const testHTTPAddress = "http://a.test.dom:123/at/some/path"

type args struct {
config configmodels.Exporter
httpAddress string
headers map[string]string
timeout time.Duration
}
type args struct {
config configmodels.Exporter
httpAddress string
headers map[string]string
timeout time.Duration
}

func TestNew(t *testing.T) {
tests := []struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one also is a single test case, let's remove the tests. It can be added later if there are multiple similar tests.

name string
args args
wantErr bool
name string
args args
}{
{
name: "empty_exporterName",
args: args{
config: nil,
httpAddress: testHTTPAddress,
},
wantErr: true,
},
{
name: "createExporter",
args: args{
Expand All @@ -57,21 +49,39 @@ func TestNew(t *testing.T) {
},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := New(tt.args.config, tt.args.httpAddress, tt.args.headers, tt.args.timeout)
if (err != nil) != tt.wantErr {
t.Errorf("New() error = %v, wantErr %v", err, tt.wantErr)
return
}

if got == nil {
return
}
assert.NoError(t, err, "nil config")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The third parameter of NoError looks incorrect to me. If I remember correctly it is the message to print when assertion fails. This assertion is not about config, why does it print "nil config"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, it is a wrong usage. Fixed

assert.NotNil(t, got)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is better to use require.NotNil here because got is used below and will crash the test when it is nil (assert does not stop test execution, require does).


// This is expected to fail.
err = got.ConsumeTraceData(context.Background(), consumerdata.TraceData{})
assert.Error(t, err)
})
}
}

func TestNewFails(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly speaking there was no need to split TestNewFails as separate function to be able to use assert, but this looks OK to me and probably is a bit clearer than having positive and negative cases in one function so let's keep it.

tests := []struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there is only one test here the value of having an array of test definitions is unclear here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assumed that other cases could be added here in the future. But you're right, for now there is only one. So I fixed this test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use the principle of YAGNI here. :-)
We generally aim to have minimum amount of code that serves the purpose.

name string
args args
}{
{
name: "empty_exporterName",
args: args{
config: nil,
httpAddress: testHTTPAddress,
},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := New(tt.args.config, tt.args.httpAddress, tt.args.headers, tt.args.timeout)
assert.EqualError(t, err, "nil config")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes all test cases in tests (if we had more than one) will return the same error message. This may not be true. Generally unclear why we have tests array and loop through it if it is one element.

assert.Nil(t, got)
})
}
}
61 changes: 38 additions & 23 deletions exporter/jaeger/jaegerthrifthttpexporter/factory_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,66 +66,81 @@ func TestCreateInstanceViaFactory(t *testing.T) {

func TestFactory_CreateTraceExporter(t *testing.T) {
tests := []struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, tests with single element.

name string
config *Config
wantErr bool
name string
config *Config
}{
{
name: "empty_url",
name: "create_instance",
config: &Config{
ExporterSettings: configmodels.ExporterSettings{
TypeVal: typeStr,
NameVal: typeStr,
},
URL: "http://some.other.location/api/traces",
Headers: map[string]string{
"added-entry": "added value",
"dot.test": "test",
},
Timeout: 2 * time.Second,
},
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
f := &Factory{}
te, err := f.CreateTraceExporter(zap.NewNop(), tt.config)
assert.NoError(t, err)
assert.NotNil(t, te)
})
}
}

func TestFactory_CreateTraceExporterFails(t *testing.T) {
tests := []struct {
name string
config *Config
errorMessage string
}{
{
name: "invalid_url",
name: "empty_url",
config: &Config{
ExporterSettings: configmodels.ExporterSettings{
TypeVal: typeStr,
NameVal: typeStr,
},
URL: "localhost:123",
},
wantErr: true,
errorMessage: "\"jaeger_thrift_http\" config requires a valid \"url\": parse : empty url",
},
{
name: "negative_duration",
name: "invalid_url",
config: &Config{
ExporterSettings: configmodels.ExporterSettings{
TypeVal: typeStr,
NameVal: typeStr,
},
Timeout: -2 * time.Second,
URL: ".localhost:123",
},
wantErr: true,
errorMessage: "\"jaeger_thrift_http\" config requires a valid \"url\": parse .localhost:123: invalid URI for request",
},
{
name: "create_instance",
name: "negative_duration",
config: &Config{
ExporterSettings: configmodels.ExporterSettings{
TypeVal: typeStr,
NameVal: typeStr,
},
URL: "http://some.other.location/api/traces",
Headers: map[string]string{
"added-entry": "added value",
"dot.test": "test",
},
Timeout: 2 * time.Second,
URL: "localhost:123",
Timeout: -2 * time.Second,
},
errorMessage: "\"jaeger_thrift_http\" config requires a positive value for \"timeout\"",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
f := &Factory{}
_, err := f.CreateTraceExporter(zap.NewNop(), tt.config)
if (err != nil) != tt.wantErr {
t.Errorf("Factory.CreateTraceExporter() error = %v, wantErr %v", err, tt.wantErr)
return
}
te, err := f.CreateTraceExporter(zap.NewNop(), tt.config)
assert.EqualError(t, err, tt.errorMessage)
assert.Nil(t, te)
})
}
}
6 changes: 4 additions & 2 deletions exporter/loggingexporter/factory_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,16 @@ func TestCreateMetricsExporter(t *testing.T) {
factory := &Factory{}
cfg := factory.CreateDefaultConfig()

_, err := factory.CreateMetricsExporter(zap.NewNop(), cfg)
me, err := factory.CreateMetricsExporter(zap.NewNop(), cfg)
assert.Nil(t, err)
assert.NotNil(t, me)
}

func TestCreateTraceExporter(t *testing.T) {
factory := &Factory{}
cfg := factory.CreateDefaultConfig()

_, err := factory.CreateTraceExporter(zap.NewNop(), cfg)
te, err := factory.CreateTraceExporter(zap.NewNop(), cfg)
assert.Nil(t, err)
assert.NotNil(t, te)
}
20 changes: 8 additions & 12 deletions exporter/loggingexporter/logging_exporter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,28 +28,24 @@ import (

func TestLoggingTraceExporterNoErrors(t *testing.T) {
lte, err := NewTraceExporter(&configmodels.ExporterSettings{}, zap.NewNop())
if err != nil {
t.Fatalf("Wanted nil got %v", err)
}
assert.NotNil(t, lte)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to use require instead of assert otherwise the test will crash if nil is returned. Same in other similar places.

assert.NoError(t, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


td := consumerdata.TraceData{
Spans: make([]*tracepb.Span, 7),
}
if err := lte.ConsumeTraceData(context.Background(), td); err != nil {
t.Fatalf("Wanted nil got %v", err)
}
assert.NoError(t, lte.ConsumeTraceData(context.Background(), td))
assert.NoError(t, lte.Shutdown())
}

func TestLoggingMetricsExporterNoErrors(t *testing.T) {
lme, err := NewMetricsExporter(&configmodels.ExporterSettings{}, zap.NewNop())
if err != nil {
t.Fatalf("Wanted nil got %v", err)
}
assert.NotNil(t, lme)
assert.NoError(t, err)

md := consumerdata.MetricsData{
Metrics: make([]*metricspb.Metric, 7),
}
if err := lme.ConsumeMetricsData(context.Background(), md); err != nil {
t.Fatalf("Wanted nil got %v", err)
}
assert.NoError(t, lme.ConsumeMetricsData(context.Background(), md))
assert.NoError(t, lme.Shutdown())
}