refactor: remove uses of testify by ywwg · Pull Request #38 · prometheus/otlptranslator

ywwg · 2025-06-10T19:09:55Z

Fixes #37

pellared · 2025-06-10T19:21:33Z

I can review it tomorrow or on Thursday.
At quick glance you can look at following recommendations:

ywwg · 2025-06-11T13:06:35Z

I'll squash down with DCO before merge

ArthurSens

Thanks, just one comment and it should be good to go!

ArthurSens · 2025-06-11T13:08:21Z

.golangci.yml

    - predeclared
    - revive
    - sloglint
-    - testifylint


Our golangci-lint configuration is synchronized with prometheus/prometheus (See #36)

So whatever changes you're making, it will be reverted 😬

ok well I guess that's fine, it'll pass :)

ArthurSens · 2025-06-11T13:09:59Z

Ah, I was about to merge #35 and realized it will create merge conflicts. Do you have a preferred order to merge these two?

ywwg · 2025-06-11T13:11:29Z

go do yours first

Copilot

Pull Request Overview

This PR removes the use of the testify library by replacing require assertions with standard testing idioms and cleans up the module dependency on testify.

Tests now use inline if checks with t.Errorf instead of require.*.
Added strings import for manual Contains checks.
Updated go.mod to drop the testify requirement.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
normalize_label_test.go	Removed `testify/require`, replaced with manual `if` + `t.Errorf`
metric_namer_test.go	Ditto for `require.Equal` and `require.Contains`, added `strings`
go.mod	Removed `github.com/stretchr/testify` from `require` block

Comments suppressed due to low confidence (1)

normalize_label_test.go:44

[nitpick] The inline if checks for equality are duplicated across tests; consider extracting a small helper (with t.Helper()) to reduce repetition and improve readability.

if utf8Allowed {

normalize_label_test.go

metric_namer_test.go

aknuds1

Generally LGTM, but please see my question.

metric_namer_test.go

Fixes #37 Signed-off-by: Owen Williams <owen.williams@grafana.com>

pellared

Minor nit comments. Thanks

pellared · 2025-06-11T13:48:43Z

metric_namer_test.go

 			// Build metric name using MetricNamer
 			gotMetricName := tt.namer.Build(tt.metric)
-			require.Equal(t, tt.expectedMetricName, gotMetricName)
+			if tt.expectedMetricName != gotMetricName {


nit: you can rename expected to want

pellared · 2025-06-11T13:49:04Z

metric_namer_test.go

 			gotMetricName := tt.namer.Build(tt.metric)
-			require.Equal(t, tt.expectedMetricName, gotMetricName)
+			if tt.expectedMetricName != gotMetricName {
+				t.Errorf("namer.Build(%v), got %q, want %q", tt.metric, gotMetricName, tt.expectedMetricName)


Suggested change

t.Errorf("namer.Build(%v), got %q, want %q", tt.metric, gotMetricName, tt.expectedMetricName)

t.Errorf("namer.Build(%q) = %q, want %q", tt.metric, gotMetricName, tt.expectedMetricName)

pellared · 2025-06-11T13:49:41Z

metric_namer_test.go

 				gotUnitName := unitNamer.Build(tt.metric.Unit)
-				require.Equal(t, tt.expectedUnitName, gotUnitName)
+				if tt.expectedUnitName != gotUnitName {
+					t.Errorf("unitNamer.Build(%q), got %q, want %q", tt.metric.Unit, gotUnitName, tt.expectedUnitName)


Suggested change

t.Errorf("unitNamer.Build(%q), got %q, want %q", tt.metric.Unit, gotUnitName, tt.expectedUnitName)

t.Errorf("unitNamer.Build(%q) = %q, want %q", tt.metric.Unit, gotUnitName, tt.expectedUnitName)

pellared · 2025-06-11T13:50:55Z

metric_namer_test.go

-					require.Contains(t, gotMetricName, gotUnitName,
-						"Metric name '%s' should contain unit name '%s' when WithMetricSuffixes=true",
-						gotMetricName, gotUnitName)
+					if !strings.Contains(gotMetricName, gotUnitName) {


We can simply add && !strings.Contains(gotMetricName, gotUnitName) to line 1119
I also think that gotUnitName != "" can be omitted.
Then we would have

if tt.namer.WithMetricSuffixes && !strings.Contains(gotMetricName, gotUnitName) {

pellared · 2025-06-11T13:51:43Z

metric_namer_test.go

-						"Metric name '%s' should contain unit name '%s' when WithMetricSuffixes=true",
-						gotMetricName, gotUnitName)
+					if !strings.Contains(gotMetricName, gotUnitName) {
+						t.Errorf("Metric name '%q' should contain unit name '%s' when WithMetricSuffixes=true", gotMetricName, gotUnitName)


Suggested change

t.Errorf("Metric name '%q' should contain unit name '%s' when WithMetricSuffixes=true", gotMetricName, gotUnitName)

t.Errorf("Metric name %q should contain unit name %q when WithMetricSuffixes=true", gotMetricName, gotUnitName)

pellared · 2025-06-11T13:51:58Z

normalize_label_test.go

 				if utf8Allowed {
-					require.Equal(t, test.label, result)
+					if test.label != result {
+						t.Errorf("labelNamer.Build(%q), got %q, want %q", test.label, result, test.label)


Suggested change

t.Errorf("labelNamer.Build(%q), got %q, want %q", test.label, result, test.label)

t.Errorf("labelNamer.Build(%q) = %q, want %q", test.label, result, test.label)

pellared · 2025-06-11T13:52:11Z

normalize_label_test.go

 				} else {
-					require.Equal(t, test.expected, result)
+					if test.expected != result {
+						t.Errorf("labelNamer.Build(%q), got %q, want %q", test.label, result, test.expected)


Suggested change

t.Errorf("labelNamer.Build(%q), got %q, want %q", test.label, result, test.expected)

t.Errorf("labelNamer.Build(%q) = %q, want %q", test.label, result, test.expected)

ywwg force-pushed the owilliams/untestify branch from d5437cf to d9c2e25 Compare June 10, 2025 19:10

ywwg mentioned this pull request Jun 10, 2025

Remove usage of testify #37

Closed

ywwg marked this pull request as ready for review June 11, 2025 13:06

ywwg requested review from ArthurSens, aknuds1 and jesusvazquez as code owners June 11, 2025 13:06

ArthurSens reviewed Jun 11, 2025

View reviewed changes

aknuds1 requested a review from Copilot June 11, 2025 13:34

Copilot AI reviewed Jun 11, 2025

View reviewed changes

normalize_label_test.go Outdated Show resolved Hide resolved

metric_namer_test.go Outdated Show resolved Hide resolved

aknuds1 reviewed Jun 11, 2025

View reviewed changes

metric_namer_test.go Outdated Show resolved Hide resolved

metric_namer_test.go Outdated Show resolved Hide resolved

metric_namer_test.go Outdated Show resolved Hide resolved

aknuds1 approved these changes Jun 11, 2025

View reviewed changes

refactor: remove uses of testify

70ddaf5

Fixes #37 Signed-off-by: Owen Williams <owen.williams@grafana.com>

ywwg force-pushed the owilliams/untestify branch from bb7c828 to 70ddaf5 Compare June 11, 2025 13:45

ywwg merged commit 61633f4 into main Jun 11, 2025
8 checks passed

ywwg deleted the owilliams/untestify branch June 11, 2025 13:48

pellared approved these changes Jun 11, 2025

View reviewed changes

pellared mentioned this pull request Jun 13, 2025

refactor unit tests to follow Go guidelines #40

Merged

	t.Errorf("namer.Build(%v), got %q, want %q", tt.metric, gotMetricName, tt.expectedMetricName)
	t.Errorf("namer.Build(%q) = %q, want %q", tt.metric, gotMetricName, tt.expectedMetricName)

	t.Errorf("unitNamer.Build(%q), got %q, want %q", tt.metric.Unit, gotUnitName, tt.expectedUnitName)
	t.Errorf("unitNamer.Build(%q) = %q, want %q", tt.metric.Unit, gotUnitName, tt.expectedUnitName)

	t.Errorf("Metric name '%q' should contain unit name '%s' when WithMetricSuffixes=true", gotMetricName, gotUnitName)
	t.Errorf("Metric name %q should contain unit name %q when WithMetricSuffixes=true", gotMetricName, gotUnitName)

	t.Errorf("labelNamer.Build(%q), got %q, want %q", test.label, result, test.label)
	t.Errorf("labelNamer.Build(%q) = %q, want %q", test.label, result, test.label)

Conversation

ywwg commented Jun 10, 2025

Uh oh!

pellared commented Jun 10, 2025

Uh oh!

ywwg commented Jun 11, 2025

Uh oh!

ArthurSens left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArthurSens commented Jun 11, 2025

Uh oh!

ywwg commented Jun 11, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

aknuds1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pellared left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants