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

fix:(go/core/tracing) doing enhancements for unit test in span data #1845

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

Conversation

falonso81
Copy link
Contributor

Description here... Help the reviewer by:

Checklist (if applicable):

func ensureAllSpanDataFieldsArePresent(t *testing.T, data Data, jsonMap map[string]any) {
spans, ok := jsonMap["spans"].(map[string]any)
if !ok {
t.Errorf("expected 'spans' to be a map, but got %T", jsonMap["spans"])
Copy link
Contributor

@hugoaguirre hugoaguirre Feb 5, 2025

Choose a reason for hiding this comment

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

Instead of doing t.Errorf(); return you can use t.Fatalf()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did the change, but we get rid of the t testing

go/core/tracing/store_test.go Outdated Show resolved Hide resolved
@hugoaguirre hugoaguirre linked an issue Feb 5, 2025 that may be closed by this pull request
2 tasks
@falonso81 falonso81 requested a review from apascal07 February 6, 2025 16:08
@apascal07
Copy link
Collaborator

I'm not sure how much value ensureAllSpanDataFieldsArePresent is adding here. We're essentially testing whether the JSON marshaling logic is working -- this should be well tested by Go core. The existing test reads the same trace.json file and compares the wire formats of both. What we want additionally (to address the linked issue) is a test of specific trace span labels -- something that we expect to be consistent between runtimes. I would look at a freshly generated trace span from JS and make sure that we are applying the same labels in Go and add a test for that (e.g. like the issue mentions, genkit:metadata:subtype).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

[Go] missing tracing metadata
3 participants