Improves TestChunkedCollector.

sourcegraph / appdash

Application tracing system for Go, based on Google's Dapper.

https://sourcegraph.com

Other

1.72k stars 137 forks source link

Improves TestChunkedCollector. #104

Closed chris-ramon closed 8 years ago

chris-ramon commented 8 years ago

Details

Issue: #101

Improves TestChunkedCollector - it does intermittently fails becase reflect.DeepEqual cares about element position when comparing, so expected & actual variables being compare sometimes might have the same elements but in different order.
[x] Adds type byTraceID which we do use to sort []*wire.CollectPacket within TestChunkedCollector.

slimsag commented 8 years ago

Awesome! I left some minor comments inline. Also, I queued a few builds and it does indeed look like this fixed the problem. Thanks for tracking this down!

chris-ramon commented 8 years ago

@slimsag thanks for reviewing this one, I def agree on the comments you mentioned above, I've sent the updates.

dmitshur commented 8 years ago

I have a question because I'm not familiar with the code.

Is the order of collected packets non-deterministic? I see that you're sorting a slice. If it's deterministic, isn't it possible to simply arrange the "want" slice to have the same order as the "got", removing the need for dynamic sorting?

If the order is indeed non-deterministic, then this solution is good.

slimsag commented 8 years ago

Thanks @chris-ramon -- LGTM.

@shurcooL yeah, the order is non-deterministic. We were seeing builds fail randomly (see #101) and not deterministically. Out of my curiosity, and for your record, I fact checked. The reason this happens is because ChunkedCollector.Collect uses a map for internally storing these. You can see that here (look for pendingBySpanID): https://sourcegraph.com/github.com/sourcegraph/appdash@master/.GoPackage/github.com/sourcegraph/appdash/.def/ChunkedCollector/Collect

dmitshur commented 8 years ago

I understand and I've confirmed that's right, thanks. :+1: