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.

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

Add telemetry/test package #1194

Merged
merged 4 commits into from
Oct 15, 2024
Merged

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Oct 14, 2024

This package is used to verify the encode/decode of the telemetry pkg is compliant with pdata (i.e. OTLP). It is added as a separate module to isolate the pdata dependencies from telemetry which is explicitly designed to not include these dependencies.

This package is used to verify the encode/decode of the telemetry pkg is
compliant with pdata (i.e. OTLP).
@MrAlias MrAlias force-pushed the telemetry-test-pkg branch from efe7c8d to 71a4186 Compare October 14, 2024 18:53
@MrAlias MrAlias marked this pull request as ready for review October 14, 2024 18:58
@MrAlias MrAlias requested a review from a team as a code owner October 14, 2024 18:58
Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

How about adding TestJSONMarshalRoundtrip test (to avoid bugs like this open-telemetry/opentelemetry-go#1273) which does something like the following:

func TestJSONMarshalRoundtrip(t *testing.T) {
  data, err := json.Marshal(traces)
  require.NoError(t, err)

  var got telemetry.Traces
  err := json.Unmarshal(data, &got)
  require.NoError(t, err)

  assert.Equal(t, traces, got)
}

It could added to tests in telemetry package. However, adding it here is easier as we already have "testdata" in form or traces field.
This comment can be addressed in a separate PR.

sdk/telemetry/test/conversion_test.go Outdated Show resolved Hide resolved
sdk/telemetry/test/conversion_test.go Outdated Show resolved Hide resolved
@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 14, 2024

How about adding TestJSONMarshalRoundtrip test which does something like the following:

func TestJSONMarshalRoundtrip(t *testing.T) {
  data, err := json.Marshal(traces)
  require.NoError(t, err)

  var got telemetry.Traces
  err := json.Unmarshal(data, &got)
  require.NoError(t, err)

  assert.Equal(t, traces, got)
}

It could added to tests in telemetry package. However, adding it here is easier as we already have "testdata" in form or traces field. This comment can be addressed in a separate PR.

I'm looking at adding this in a separate PR presently.

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

👍

@MrAlias
Copy link
Contributor Author

MrAlias commented Oct 15, 2024

How about adding TestJSONMarshalRoundtrip test which does something like the following:

func TestJSONMarshalRoundtrip(t *testing.T) {
  data, err := json.Marshal(traces)
  require.NoError(t, err)

  var got telemetry.Traces
  err := json.Unmarshal(data, &got)
  require.NoError(t, err)

  assert.Equal(t, traces, got)
}

It could added to tests in telemetry package. However, adding it here is easier as we already have "testdata" in form or traces field. This comment can be addressed in a separate PR.

I'm looking at adding this in a separate PR presently.

#1196

MrAlias and others added 3 commits October 15, 2024 07:57
Co-authored-by: Robert Pająk <pellared@hotmail.com>
The collector issue blocking this was resolved.
@MrAlias MrAlias merged commit 55dc1e9 into open-telemetry:main Oct 15, 2024
26 checks passed
@MrAlias MrAlias deleted the telemetry-test-pkg branch October 15, 2024 15:27
@MrAlias MrAlias added this to the v0.16.0-alpha milestone Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants