Skip to content

telemetry: add tests for trace id and span id encoding #2272

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

yumosx
Copy link
Contributor

@yumosx yumosx commented May 4, 2025

add tests for trace id and span id encoding.

@yumosx yumosx requested a review from a team as a code owner May 4, 2025 13:17
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing tests for the errors cases where the IDs are invalid lengths or they do not contain valid hex encoded data.

Copy link
Contributor

Choose a reason for hiding this comment

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

@yumosx are you able to address the errors testing? Is this something you could add to this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

@yumosx just checking were you able to address the errors? If so please feel free to update/rebase the branch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, My oversight delayed this . I'll fix it promptly.

Copy link
Contributor

@grcevski grcevski left a comment

Choose a reason for hiding this comment

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

This looks good to me now! I wonder if we can add tests for IsEmpty() too, but perhaps that can be done in another PR.

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.

4 participants