Improved tests for a logging exporter and jaeger exporter #377

agneum · 2019-10-03T06:50:13Z

Some improvements of tests for an issue #266

tigrannajaryan · 2019-10-03T13:27:16Z

@agneum thank you for contributing! Please make sure the build passes and the maintainers will review the PR.

agneum · 2019-10-03T14:47:10Z

tigrannajaryan · 2019-10-03T16:39:28Z

exporter/jaeger/jaegerthrifthttpexporter/exporter_test.go

-				return
-			}
+			assert.NoError(t, err, "nil config")
+			assert.NotNil(t, got)


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).

tigrannajaryan · 2019-10-03T16:46:37Z

exporter/jaeger/jaegerthrifthttpexporter/exporter_test.go


 			// This is expected to fail.
 			err = got.ConsumeTraceData(context.Background(), consumerdata.TraceData{})
 			assert.Error(t, err)
 		})
 	}
 }
+
+func TestNewFails(t *testing.T) {


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.

tigrannajaryan · 2019-10-03T16:47:57Z

exporter/jaeger/jaegerthrifthttpexporter/exporter_test.go


 			// This is expected to fail.
 			err = got.ConsumeTraceData(context.Background(), consumerdata.TraceData{})
 			assert.Error(t, err)
 		})
 	}
 }
+
+func TestNewFails(t *testing.T) {
+	tests := []struct {


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

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.

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

tigrannajaryan · 2019-10-03T16:48:45Z

exporter/jaeger/jaegerthrifthttpexporter/exporter_test.go

+	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")


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.

tigrannajaryan · 2019-10-03T16:49:30Z

exporter/jaeger/jaegerthrifthttpexporter/factory_test.go

@@ -66,66 +66,81 @@ func TestCreateInstanceViaFactory(t *testing.T) {

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


Same here, tests with single element.

tigrannajaryan · 2019-10-03T16:49:50Z

exporter/loggingexporter/logging_exporter_test.go

-		t.Fatalf("Wanted nil got %v", err)
-	}
+	assert.NotNil(t, lte)
+	assert.NoError(t, err)


tigrannajaryan · 2019-10-03T16:50:17Z

exporter/loggingexporter/logging_exporter_test.go

-	if err != nil {
-		t.Fatalf("Wanted nil got %v", err)
-	}
+	assert.NotNil(t, lte)


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

tigrannajaryan · 2019-10-03T16:53:30Z

@agneum thank you for working on this. Please make sure assert or require is chosen according to what is needed in the particular piece of code.

pjanotti · 2019-10-03T19:57:30Z

kicking another build to see if we get coverage numbers for this PR

codecov-io · 2019-10-03T20:13:47Z

Codecov Report

Merging #377 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #377   +/-   ##
=======================================
  Coverage   74.81%   74.81%           
=======================================
  Files         115      115           
  Lines        6885     6885           
=======================================
  Hits         5151     5151           
  Misses       1478     1478           
  Partials      256      256

Impacted Files	Coverage Δ
service/builder/receivers_builder.go	`82.2% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e505d5...18a976d. Read the comment docs.

…, simplify some tests

agneum · 2019-10-04T07:34:41Z

@tigrannajaryan Thank you for a review. I've fixed all mistakes. Could you please check it one more time?

tigrannajaryan · 2019-10-04T14:07:16Z

exporter/jaeger/jaegerthrifthttpexporter/exporter_test.go

+	timeout     time.Duration
+}
+
+func TestNew(t *testing.T) {
 	tests := []struct {


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

tigrannajaryan · 2019-10-04T14:08:32Z

exporter/jaeger/jaegerthrifthttpexporter/exporter_test.go

-			if got == nil {
-				return
-			}
+			assert.NoError(t, err, "nil config")


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"?

Indeed, it is a wrong usage. Fixed

tigrannajaryan

LGTM
Thank you @agneum

… memory limit (open-telemetry#377)

… OpenTelemetry, use submodules for CMake deps (open-telemetry#377)

improve exporter tests for logging and jaeger

6c182b6

agneum requested review from bogdandrutu, flands, owais, pjanotti, songy23 and tigrannajaryan as code owners October 3, 2019 06:50

fix fmt

9e9ac29

tigrannajaryan reviewed Oct 3, 2019

View reviewed changes

replace the assert package with the require package in exporter tests…

af4feea

…, simplify some tests

tigrannajaryan reviewed Oct 4, 2019

View reviewed changes

fix a test for a jaeger thrift http exporter

18a976d

tigrannajaryan approved these changes Oct 4, 2019

View reviewed changes

songy23 approved these changes Oct 5, 2019

View reviewed changes

tigrannajaryan merged commit 6db8fae into open-telemetry:master Oct 7, 2019

hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023

nit: use a strict greater than so folks can set ballast as exactly 2x…

343127d

… memory limit (open-telemetry#377)

Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024

Build tools: build configurations, vcpkg support, vcpkg port file for…

2a516ad

… OpenTelemetry, use submodules for CMake deps (open-telemetry#377)

swiatekm pushed a commit to swiatekm/opentelemetry-collector that referenced this pull request Oct 9, 2024

fix: logsCollection CRI regex (open-telemetry#377)

d77de49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved tests for a logging exporter and jaeger exporter #377

Improved tests for a logging exporter and jaeger exporter #377

agneum commented Oct 3, 2019

tigrannajaryan commented Oct 3, 2019

agneum commented Oct 3, 2019

tigrannajaryan Oct 3, 2019

tigrannajaryan Oct 3, 2019

tigrannajaryan Oct 3, 2019

agneum Oct 4, 2019

tigrannajaryan Oct 4, 2019

tigrannajaryan Oct 3, 2019

tigrannajaryan Oct 3, 2019

tigrannajaryan Oct 3, 2019

tigrannajaryan Oct 3, 2019

tigrannajaryan commented Oct 3, 2019

pjanotti commented Oct 3, 2019

codecov-io commented Oct 3, 2019 •

edited

Loading

agneum commented Oct 4, 2019

tigrannajaryan Oct 4, 2019

tigrannajaryan Oct 4, 2019

agneum Oct 4, 2019

tigrannajaryan left a comment

		@@ -66,66 +66,81 @@ func TestCreateInstanceViaFactory(t *testing.T) {

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

Improved tests for a logging exporter and jaeger exporter #377

Improved tests for a logging exporter and jaeger exporter #377

Conversation

agneum commented Oct 3, 2019

tigrannajaryan commented Oct 3, 2019

agneum commented Oct 3, 2019

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

tigrannajaryan commented Oct 3, 2019

pjanotti commented Oct 3, 2019

codecov-io commented Oct 3, 2019 • edited Loading

Codecov Report

agneum commented Oct 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigrannajaryan left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 3, 2019 •

edited

Loading