-
Notifications
You must be signed in to change notification settings - Fork 114
Conversation
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 would also make the opentracing_mocktracer configurable, so for e.g. embedded systems you can skip the build of the mocktracer
forget this comment, I see it is added in one of the commits
@yurishkuro, @isaachier -- are you guys ok with this PR? It doesn't make any changes to the actual API. |
install(TARGETS opentracing_mocktracer EXPORT OpenTracingTargets | ||
LIBRARY DESTINATION lib | ||
ARCHIVE DESTINATION lib) | ||
endif(BUILD_SHARED_LIBS) |
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.
Not asking you to change this, just an FYI, the CMake docs seem to be wrong about this point. The expression need not/should not be included in endif
arguments. None of the CMake modules follow that pattern.
include(CTest) | ||
if(BUILD_TESTING) | ||
add_subdirectory(test) | ||
endif() |
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.
Overall, great work on the CMake stuff :). Very few projects get this right.
This PR adds a mocktracer library similar in design to that included in the other OpenTracing language bindings (for example, Go). It will make it easier for users to test their instrumentation code.
Additionally, I switched all the testing code to use catch2 since it was outgrowing the existing testing framework.