Add method finishedTraces to MockTracer.

[qudongfang]

Resolves #277

[sjoerdtalsma]

No other comments, thanks for the change

[qudongfang]

cc @pavolloffay

[whiskeysierra]

Alternatively it could be made read-only/unmodifiable using Collections.unmodifiable*. Has to be done deep though, i.e. to all contained maps as well.

On Mon, Aug 12, 2019, 11:53 Pavol Loffay notifications@github.com wrote:

@pavolloffay commented on this pull request.

In opentracing-mock/src/main/java/io/opentracing/mock/MockTracer.java https://github.com/opentracing/opentracing-java/pull/359#discussion_r312849426 :

@@ -95,6 +93,28 @@ public synchronized void reset() { return new ArrayList<>(this.finishedSpans); }

• /**
• • @return a copy of all finish()ed Traces(Spans) started by this MockTracer grouped by traceId and spanId in HashMap format.
• */
• public Map<String, Map<String, Span>> finishedTraces() {

It should return a MockSpan not span

In opentracing-mock/src/main/java/io/opentracing/mock/MockTracer.java https://github.com/opentracing/opentracing-java/pull/359#discussion_r312849522 :

@@ -95,6 +93,28 @@ public synchronized void reset() { return new ArrayList<>(this.finishedSpans); }

• /**
• • @return a copy of all finish()ed Traces(Spans) started by this MockTracer grouped by traceId and spanId in HashMap format.

It does not return a copy

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/opentracing/opentracing-java/pull/359?email_source=notifications&email_token=AADI7HN6PIL7O3VMZYL4UULQEEXLDA5CNFSM4IK6KQ32YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBHME4A#pullrequestreview-273597040, or mute the thread https://github.com/notifications/unsubscribe-auth/AADI7HI6HEADKEMB3C2URJDQEEXLDANCNFSM4IK6KQ3Q .

[qudongfang]

Alternatively it could be made read-only/unmodifiable using Collections.unmodifiable*

Thanks for your suggestion. I am not sure if we really need it to be a copy or read-only/unmodifiable.

[pavolloffay]

+1 on unmodifiable

[sjoerdtalsma]

Not sure on unmodifiable. Each call returns a new Map structure anyway.\ Any pressing reasons for the unmodifiable, seeing that the map values must become unmodifiable as well?

[carlosalberto]

Agreed, I don't think we have to add the unmodifiable part. For example, finishedSpans() is defined as:

    public synchronized List<MockSpan> finishedSpans() {
        return new ArrayList<>(this.finishedSpans);
    }

[qudongfang]

+1 on unmodifiable

Done.

[whiskeysierra]

I wasn't aware that the one right returns a copy/snapshot. I'd keep consistency and do the same then.

On Wed, Aug 14, 2019, 12:14 qudongfang notifications@github.com wrote:

@qudongfang commented on this pull request.

In opentracing-mock/src/main/java/io/opentracing/mock/MockTracer.java https://github.com/opentracing/opentracing-java/pull/359#discussion_r313801693 :

  * MockTracer.reset()).

*

• @see MockTracer#reset() */ public synchronized List finishedSpans() {
• return new ArrayList<>(this.finishedSpans);
• return Collections.unmodifiableList(new ArrayList<>(this.finishedSpans));

Yes, It is a breaking change.

I think it is not right to make finishedTraces unmodifiable while finishedSpans modifiable. @pavolloffay https://github.com/pavolloffay Shall we make them all modifiable?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/opentracing/opentracing-java/pull/359?email_source=notifications&email_token=AADI7HITHDWM2SIMOEPELFDQEPLGVA5CNFSM4IK6KQ32YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBQPYXA#discussion_r313801693, or mute the thread https://github.com/notifications/unsubscribe-auth/AADI7HMQFW4IRMBA4TX7UKDQEPLGVANCNFSM4IK6KQ3Q .

[coveralls]

Coverage increased (+0.6%) to 76.586% when pulling cf6bccec957c3042bcd44ebf8813bba67e7bff7c on qudongfang:finishedTraces into 70555ed63545fd642064ea96ac92594051d5b6a6 on opentracing:master.

[carlosalberto]

Hey, thanks a lot for the updates, appreciated. It looks good now, and just left a small comment about keeping the comment regarding the returned MockSpans being a copy. After that we should be good to merge ;)

[carlosalberto]

Merged, thanks!