MockTracer passes Reference instead of MockSpanContext

[ava-joeys]

Background

I was investigating the usage of the OpenTracing API and found that the MockTracer is broken.

Problem

This line: https://github.com/opentracing/opentracing-php/blob/master/src/OpenTracing/Mock/MockTracer.php#L70 breaks due to the definition here: https://github.com/opentracing/opentracing-php/blob/master/src/OpenTracing/Mock/MockSpanContext.php#L49 You are sending a Reference object but you are defining the function to only take MockSpanContext objects.

Proposal

If you want the createAsChildOf to accept only MockSpanContexts, you will need to update the MockTracer to send $options->getReferences()[0]->getContext(). This update allows the MockTracer to not fail immediately.

Questions to address

In what capacity do you want the MockTracer to work? In other words, should the MockTracer, and the Mock implementation in general, be considered the basis for a specific implementation of the OpenTracing API? Should the different objects interact explicitly as defined by the API? Or is the MockTracer just to show the minimum classes and functions that would be required for a tracing implementation and is not explicitly designed to work? Should the Unit Tests catch this case?

[cawolf]

In what capacity do you want the MockTracer to work?

From working with both the NoopTracer and MockTracer, I would consider the NoopTracer as a drop-in replacement for actual working tracer implementations, not breaking your application and just doing nothing.

I used the MockTracer instead as base implementation for our own mock implementation, to test a library that interacts with the opentracing API (so this project), and ensuring that the library was using the API correctly, with the desired side effects.

In other words, should the MockTracer, and the Mock implementation in general, be considered the basis for a specific implementation of the OpenTracing API?

As mentioned, I would not consider it as a base for a production tracer implementation, but for a mock implementation for testing purposes.

Should the different objects interact explicitly as defined by the API?

Refering to your hinted code lines, I would consider this a bug and fix it. I did not happen to use this branch in my implementation yet, but I will soon. So I can provide a PR trying to solve that issue.

Or is the MockTracer just to show the minimum classes and functions that would be required for a tracing implementation and is not explicitly designed to work?

I wouldn't think so, as mentioned above.

Should the Unit Tests catch this case?

Yes, especially after finding a bug at that lines of code, I would highly recommend and cover it with a test.

@jcchavezs I'm going to provide a PR to fix the API break in the MockTracer as hinted

[jcchavezs]

In what capacity do you want the MockTracer to work? In other words, should the MockTracer, and the Mock implementation in general, be considered the basis for a specific implementation of the OpenTracing API?

Mock tracer is a reference implementation and also helps for testing purposes.

Should the different objects interact explicitly as defined by the API? Or is the MockTracer just to show the minimum classes and functions that would be required for a tracing implementation and is not explicitly designed to work?

MockTracer should be fully functional in terms of API matters, this means that plugin mocktracer or zipkin-opentracing (for example) should not break the app (remember, mock tracer won't report data).

Should the Unit Tests catch this case? Yes, tests in mocktracer should have catch this issue. Would you fix it with a PR @cawolf ?

[cawolf]

Sure, PR is ready :)

[cawolf]

Relates to #93