-
Notifications
You must be signed in to change notification settings - Fork 308
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
HPCC-33297 Add Jtrace isValid Tests #19450
HPCC-33297 Add Jtrace isValid Tests #19450
Conversation
@jpmcmu please review |
Signed-off-by: Rodrigo Pastrana <[email protected]>
0d15cc2
to
e66d445
Compare
Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-33163 Jirabot Action Result: |
@jpmcmu please review |
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.
@rpastrana looks good
@ghalliday / @jakesmith please merge |
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.
@rpastrana change looks good. One comment/suggestion.
Please let me know if you want me to merge as-is
|
||
const char * nullSpanChildTraceID = nullSpanChild->queryTraceId(); | ||
CPPUNIT_ASSERT_EQUAL_MESSAGE("Unexpected nullspanChild TraceID==nullptr", false, nullSpanChildTraceID==nullptr); | ||
CPPUNIT_ASSERT_EQUAL_MESSAGE("Unexpected nullspanChild TraceID length", strlen("00000000000000000000000000000000"), strlen(nullSpanChildTraceID)); |
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.
Is there any advantage comparing the length first?
can you use
CPPUNIT_ASSERT_EQUAL("Unexpected nullspanChild TraceID", std::string(nullSpanChildTraceId), std::string("00000000000000000000"));
and also get details of the wrong value?
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.
yeah, I wasn't going for efficiency here. I wanted to explicitly call out the length expectation, I can change if you think its worthwhile
@ghalliday I vote we merge as is |
Jirabot Action Result: |
Type of change:
Checklist:
Smoketest:
Testing: