WIP integration-test fixtures should use internal model

[burmanm]

Integration-test fixtures should use internal model and not domain model for comparison purposes since these storage backends don't deal with the UI data models.

This commit is also available in the PR #760 but I figured another PR would be suitable for this. Can be closed if merging with the #760 is enough.

Signed-off-by: Michael Burman [email protected]

[coveralls]

Coverage remained the same at 100.0% when pulling 095a247 on burmanm:fixture_fix into c7891a4 on jaegertracing:master.

[yurishkuro]

I added a WIP label since the ES unit tests are failing

[burmanm]

This is more tricky, since the ES storage returns on purpose the wrong one..

	// Convert model.Span into json.Span
	jsonSpan := json.FromDomainEmbedProcess(span)

@pavolloffay Got any comments ?

[pavolloffay]

I am not sure if I understand why it fails. I think the integration test should unmarshall a fixture into model (not model/json), pass it to the storage and then get it from the reader and compare (two model.Trace).

[burmanm]

@pavolloffay Well, it fails because of the mentioned trick. The modification from model -> model/json adds "information" that is not removed when you transfer it back to the model

That is, instead of having a nil references, what the domain model does is have an empty slice there. Now, in Go nil and empty slice are not the same thing and that's why the test fails.

From the user perspective it is irrelevant of course (as the behavior in normal cases is the same), but in terms of memory handling there's a small difference (empty slices have a pointer to the underlying empty data).

The differ could be modified to ignore this type of differences of course.

[pavolloffay]

Well, it fails because of the mentioned trick. The modification from model -> model/json adds "information" that is not removed when you transfer it back to the model

model/convertor/json/to_domain from json->model creates empty slices for every array. if you add the following commit tests pass pavolloffay@1ab4338 (Note that you didn't remove empty array in log.fields)

Cassandra convertor works the same way, That said the integration test would not pass either https://github.com/jaegertracing/jaeger/blob/master/plugin/storage/cassandra/spanstore/dbmodel/converter.go#L97.

Why is it needed for #760? Only for memory allocations?

[burmanm]

It's got nothing to do with memory allocations, but the testing framework. The #760 serializes the incoming object when writing and then deserializes when reading. Thus, it has to follow the internal domain model as that's what the testing framework requests (and the API uses). Thus, the path is internal model -> serialized form -> deserialization -> internal model. It's not in the json domain model at any point and as such shouldn't follow those settings.

The #760 is not restricted to storing only the JSON from writes, but instead will support the protobuf serialization when that's introduced. Thus, it has to follow strictly the internal model definitions - not the domain model to outside. You will run into same issues with any storage that does not store the original JSON as JSON.

And since these tests should test the internal behavior (not REST API), they should follow the correct serialization rules also. If we want to be strict about these internal model serialization rules.

[pavolloffay]

@burmanm thanks for the explanation. I see your point now.

I don't have a strong opinion whether we should strictly test nil vs empty slice. It seems that all converters for DB models create empty slices. There are also plans to run the integration tests for C* #681. There is the same problem.

Also, ES implementation stores empty arrays (tags, logs) this can be adjusted by omitempty.

[burmanm]

Not relevant anymore.