Avoid nil pointer derefence panic edge case, add tests, add comments

Signed-off-by: Arianna Vespri <[email protected]>

Author: vesari

expfmt/encode_test.go

@@ -120,47 +120,47 @@ func TestEncode(t *testing.T) {
 		metric   *dto.MetricFamily
 		format   Format
 		withUnit bool
-		out      string
+		expOut   string
 	}{
-		// // 1: Untyped ProtoDelim
-		// {
-		// 	metric: metric1,
-		// 	format: FmtProtoDelim,
-		// 	out:    ,
-		// },
-		// // 2: Untyped FmtProtoCompact
-		// {
-		// 	metric: metric1,
-		// 	format: FmtProtoCompact,
-		// 	out:   ,
-		// },
-		// // 3: Untyped FmtProtoText
-		// {
-		// 	metric: metric1,
-		// 	format: FmtProtoText,
-		// 	out:   ,
-		// },
+		// 1: Untyped ProtoDelim
+		{
+			metric: metric1,
+			format: FmtProtoDelim,
+			// expOut: `\x1b\n\nfoo_metric\x18\x03\"\v*\t\tX9\xb4\xc8v\xbe\xf3?`, // does this look like the desired output?
+		},
+		// 2: Untyped FmtProtoCompact
+		{
+			metric: metric1,
+			format: FmtProtoCompact,
+			// expOut: `name:\"foo_metric\" type:UNTYPED metric:{untyped:{value:1.234}}\n`, // does this look like the desired output?
+		},
+		// 3: Untyped FmtProtoText
+		{
+			metric: metric1,
+			format: FmtProtoText,
+			// expOut: `name: \"foo_metric\"\ntype: UNTYPED\nmetric: {\n  untyped: {\n    value: 1.234\n  }\n}\n\n`, // does this look like the desired output?
+		},
 		// 4: Untyped FmtText
 		{
 			metric: metric1,
 			format: FmtText,
-			out: `# TYPE foo_metric untyped
+			expOut: `# TYPE foo_metric untyped
 foo_metric 1.234
 `,
 		},
 		// 5: Untyped FmtOpenMetrics_0_0_1
 		{
 			metric: metric1,
 			format: FmtOpenMetrics_0_0_1,
-			out: `# TYPE foo_metric unknown
+			expOut: `# TYPE foo_metric unknown
 foo_metric 1.234
 `,
 		},
 		// 6: Untyped FmtOpenMetrics_1_0_0
 		{
 			metric: metric1,
 			format: FmtOpenMetrics_1_0_0,
-			out: `# TYPE foo_metric unknown
+			expOut: `# TYPE foo_metric unknown
 foo_metric 1.234
 `,
 		},
@@ -181,7 +181,7 @@ foo_metric 1.234
 			},
 			format:   FmtOpenMetrics_0_0_1,
 			withUnit: true,
-			out: `# HELP foos_duration_seconds Duration of foos in seconds.
+			expOut: `# HELP foos_duration_seconds Duration of foos in seconds.
 # TYPE foos_duration_seconds counter
 # UNIT foos_duration_seconds seconds
 foos_duration_seconds_total 420.0
@@ -202,7 +202,7 @@ foos_duration_seconds_total 420.0
 				},
 			},
 			format: FmtOpenMetrics_1_0_0,
-			out: `# HELP foos_duration_seconds Duration of foos in seconds.
+			expOut: `# HELP foos_duration_seconds Duration of foos in seconds.
 # TYPE foos_duration_seconds counter
 foos_duration_seconds_total 420.0
 `,
@@ -213,24 +213,33 @@ foos_duration_seconds_total 420.0
 		if scenario.withUnit {
 			opts = append(opts, ToOpenMetricsWithUnit())
 		}
-		out := bytes.NewBuffer(make([]byte, 0, len(scenario.out)))
+		out := bytes.NewBuffer(make([]byte, 0, len(scenario.expOut)))
 		enc := NewEncoder(out, scenario.format, opts...)
 		err := enc.Encode(scenario.metric)
 		if err != nil {
 			t.Errorf("%d. error: %s", i, err)
 			continue
 		}
-
-		if expected, got := len(scenario.out), len(out.Bytes()); expected != got {
-			t.Errorf(
-				"%d. expected %d bytes written, got %d",
-				i, expected, got,
-			)
+		// N.B. For the first three cases, I'm maintaining the same checks as they were before, i. e. the expected output is
+		// not declared ergo no comparison with the actual output is ever performed, expect for its length being bigger than zero.
+		if i > 2 {
+			if expected, got := len(scenario.expOut), len(out.Bytes()); expected != got {
+				t.Errorf(
+					"%d. expected %d bytes written, got %d",
+					i, expected, got,
+				)
+			}
+			if expected, got := scenario.expOut, out.String(); expected != got {
+				t.Errorf(
+					"%d. expected out=%q, got %q",
+					i, expected, got,
+				)
+			}
 		}
-		if expected, got := scenario.out, out.String(); expected != got {
+		if len(out.Bytes()) == 0 {
 			t.Errorf(
-				"%d. expected out=%q, got %q",
-				i, expected, got,
+				"%d. expected output not to be empty",
+				i,
 			)
 		}
 	}

expfmt/openmetrics_create.go

@@ -27,7 +27,7 @@ import (
 	dto "github.com/prometheus/client_model/go"
 )
 
-type ToOpenMetrics struct { // TODO: reunite the comment above with its right function!!
+type ToOpenMetrics struct {
 	withUnit bool
 	// withCreated bool can be added in the future.
 }
@@ -121,7 +121,7 @@ func MetricFamilyToOpenMetrics(out io.Writer, in *dto.MetricFamily, options ...T
 	if metricType == dto.MetricType_COUNTER && strings.HasSuffix(shortName, "_total") {
 		shortName = name[:len(name)-6]
 	}
-	if toOM.withUnit && !strings.HasSuffix(shortName, fmt.Sprintf("_%s", *in.Unit)) {
+	if toOM.withUnit && in.Unit != nil && !strings.HasSuffix(shortName, fmt.Sprintf("_%s", *in.Unit)) {
 		shortName = shortName + fmt.Sprintf("_%s", *in.Unit)
 	}
 

expfmt/openmetrics_create_test.go

@@ -439,7 +439,7 @@ foos_total 42.0
 # UNIT name_seconds seconds
 `,
 		},
-		// 10: Histogram plus unit.
+		// 10: Histogram plus unit, but unit not opted in.
 		{
 			in: &dto.MetricFamily{
 				Name: proto.String("request_duration_microseconds"),
@@ -488,7 +488,7 @@ request_duration_microseconds_sum 1.7560473e+06
 request_duration_microseconds_count 2693
 `,
 		},
-		// 11: No metric unit opted in no unit in name
+		// 11: No metric, unit opted in, no unit in name.
 		{
 			in: &dto.MetricFamily{
 				Name:   proto.String("name_total"),
@@ -501,6 +501,19 @@ request_duration_microseconds_count 2693
 			out: `# HELP name_seconds doc string
 # TYPE name_seconds counter
 # UNIT name_seconds seconds
+`,
+		},
+		// 12: No metric, unit opted in, BUT unit == nil.
+		{
+			in: &dto.MetricFamily{
+				Name:   proto.String("name_total"),
+				Help:   proto.String("doc string"),
+				Type:   dto.MetricType_COUNTER.Enum(),
+				Metric: []*dto.Metric{},
+			},
+			withUnit: true,
+			out: `# HELP name doc string
+# TYPE name counter
 `,
 		},
 	}