Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

Reduce visibility of TextMapCodec.contextFromString to package scope #519

Merged
merged 8 commits into from
Aug 20, 2018
Merged

Reduce visibility of TextMapCodec.contextFromString to package scope #519

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3ad52db
Reduce visibility of TextMapCodec.contextFromString to package scope
Aug 17, 2018
a1cb911
Get rid of JaegerSpanContextTest
Aug 17, 2018
843c937
Remove outdated comment
Aug 17, 2018
8847020
Change contextAsString to package scope
Aug 20, 2018
fa973ea
Style fixes
Aug 20, 2018
986986e
Move contextAsString to public method of TextMapCodec
Aug 20, 2018
48112ed
Specify expected string output with string formatting
Aug 20, 2018
900524b
Use fixed IDs to avoid string formatting
Aug 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion jaeger-core/src/main/java/io/jaegertracing/internal/propagation/TextMapCodec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ private TextMapCodec(Builder builder) {
this.baggagePrefix = builder.baggagePrefix;
}

public static JaegerSpanContext contextFromString(String value)
static JaegerSpanContext contextFromString(String value)
throws MalformedTracerStateStringException, EmptyTracerStateStringException {
if (value == null || value.equals("")) {
throw new EmptyTracerStateStringException();
Expand Down
66 changes: 0 additions & 66 deletions jaeger-core/src/test/java/io/jaegertracing/internal/JaegerSpanContextTest.java
View file
Open in desktop

This file was deleted.

12 changes: 0 additions & 12 deletions jaeger-core/src/test/java/io/jaegertracing/internal/JaegerSpanTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,18 +208,6 @@ public void testWithoutTimestampsInaccurateClock() {
assertEquals(10, jaegerSpan.getDuration());
}

@Test
public void testSpanToString() {
JaegerSpan jaegerSpan = tracer.buildSpan("test-operation").start();
JaegerSpanContext expectedContext = jaegerSpan.context();
JaegerSpanContext actualContext = TextMapCodec.contextFromString(expectedContext.contextAsString());

assertEquals(expectedContext.getTraceId(), actualContext.getTraceId());
assertEquals(expectedContext.getSpanId(), actualContext.getSpanId());
assertEquals(expectedContext.getParentId(), actualContext.getParentId());
assertEquals(expectedContext.getFlags(), actualContext.getFlags());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing it is not equivalent. But you can change L215 to use TextMapCodec's extract method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this still applies. See testSpanToString in TextMapCodec.

Copy link
Member

@yurishkuro yurishkuro Aug 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's what I am thinking. The method is called testSpanToString. Such method should belong in SpanTest class, not the codecs. But it's not actually testing span.toString(), only spanContext.contextAsString(). So let's separate these.

  • to test span.toString(), we can simply compare with an exact literal string (including the operation name).
  • what is the contract of contextAsString()? Do we, strictly speaking, care if the output matches the output of the codec? What if we use a different codec in the future? I think it mattered more when span context had the opposite fromString() method, but now contextAsString() seems merely a dependency of span.toString(), nothing more. Or is it used elsewhere? If not, we can not test it at all (since it's covered by span.toString() test), and we can consider making it package viz.

Counter-proposal: keep the test here (since it's testing Span's behavior, not the codec), but

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what? :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, a left over

}

@Test
public void testOperationName() {
String expectedOperation = "leela";
Expand Down
57 changes: 57 additions & 0 deletions jaeger-core/src/test/java/io/jaegertracing/internal/propagation/TextMapCodecTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,15 @@

package io.jaegertracing.internal.propagation;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;

import io.jaegertracing.internal.JaegerSpan;
import io.jaegertracing.internal.JaegerSpanContext;
import io.jaegertracing.internal.exceptions.EmptyTracerStateStringException;
import io.jaegertracing.internal.exceptions.MalformedTracerStateStringException;
import java.util.Collections;
import org.junit.Test;

public class TextMapCodecTest {
Expand Down Expand Up @@ -44,4 +50,55 @@ public void testWithoutBuilder() {
assertTrue(str.contains("urlEncoding=false"));
}

@Test
public void testSpanToString() {
JaegerSpanContext expectedContext = new JaegerSpanContext(1, 2, 3, (byte) 1);
JaegerSpanContext actualContext = TextMapCodec.contextFromString(expectedContext.contextAsString());

assertEquals(expectedContext.getTraceId(), actualContext.getTraceId());
assertEquals(expectedContext.getSpanId(), actualContext.getSpanId());
assertEquals(expectedContext.getParentId(), actualContext.getParentId());
assertEquals(expectedContext.getFlags(), actualContext.getFlags());
}

@Test(expected = MalformedTracerStateStringException.class)
public void testContextFromStringMalformedException() throws Exception {
TextMapCodec.contextFromString("ff:ff:ff");
}

@Test(expected = EmptyTracerStateStringException.class)
public void testContextFromStringEmptyException() throws Exception {
TextMapCodec.contextFromString("");
}

@Test
public void testContextFromString() throws Exception {
JaegerSpanContext context = TextMapCodec.contextFromString("ff:dd:cc:4");
assertEquals(context.getTraceId(), 255);
assertEquals(context.getSpanId(), 221);
assertEquals(context.getParentId(), 204);
assertEquals(context.getFlags(), 4);
}

@Test
public void testToStringFormatsPositiveFields() {
long traceId = -10L;
long spanId = -10L;
long parentId = -10L;
byte flags = (byte) 129;

// I use MIN_VALUE because the most significant bit, and thats when
// we want to make sure the hex number is positive.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment looks like some old left-over. Doesn't seem to make any sense now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

JaegerSpanContext context = new JaegerSpanContext(traceId, spanId, parentId, flags);

context.contextAsString().split(":");

assertEquals(
"fffffffffffffff6:fffffffffffffff6:fffffffffffffff6:81", context.contextAsString());
JaegerSpanContext contextFromStr = TextMapCodec.contextFromString(context.contextAsString());
assertEquals(traceId, contextFromStr.getTraceId());
assertEquals(spanId, contextFromStr.getSpanId());
assertEquals(parentId, contextFromStr.getParentId());
assertEquals(flags, contextFromStr.getFlags());
}
}