-
Notifications
You must be signed in to change notification settings - Fork 77
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
Update tracestate duplicate keys in test_data.json #547
Conversation
pellared marked as non substantive for IPR from ash-nazg. |
test/test_data.json
Outdated
{"headers": [["traceparent", "00-12345678901234567890123456789012-1234567890123456-00"], ["tracestate", "foo=1,foo=1"]], "is_traceparent_valid": true, "is_tracestate_valid": true}, | ||
{"headers": [["traceparent", "00-12345678901234567890123456789012-1234567890123456-00"], ["tracestate", "foo=1,foo=2"]], "is_traceparent_valid": true, "is_tracestate_valid": true}, | ||
{"headers": [["traceparent", "00-12345678901234567890123456789012-1234567890123456-00"], ["tracestate", "foo=1"], ["tracestate", "foo=1"]], "is_traceparent_valid": true, "is_tracestate_valid": false}, | ||
{"headers": [["traceparent", "00-12345678901234567890123456789012-1234567890123456-00"], ["tracestate", "foo=1"], ["tracestate", "foo=2"]], "is_traceparent_valid": true, "is_tracestate_valid": false}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change is correct. For context, it seems like self_test.py writes this file. Actually, for me it also changes the next two cases to "is_tracestate_valid": true
when I run this on main
-- they are basically the same cases, just that the headers are sent as multiline headers. @pellared would you mind changing lines 47 and 48 accordingly?
@dyladan would you agree? Also, it seems the file is only ever written to, by self_test.py. Should it rather be in .gitignore
instead of being under source control?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 85899d0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follows #477
Relates to #548