Skip to content

Commit

Permalink
refactor: migrates from JUnit 4 to JUnit 5 (Jupiter) (#222)
Browse files Browse the repository at this point in the history
This migrates from JUnit 4 to JUnit 5 (Jupiter) beginning with the
following OpenRewrite command:

```bash
$ ./mvnw -U org.openrewrite.maven:rewrite-maven-plugin:run \
  -Drewrite.recipeArtifactCoordinates=org.openrewrite.recipe:rewrite-testing-frameworks:LATEST \
  -Drewrite.activeRecipes=org.openrewrite.java.testing.junit5.JUnit4to5Migration
```

After this, we had to do some work, notably as Brave needs to support
non-alpha versions of OkHttp and alpha 5 creates a classpath conflict on
Okio. Notably, I had to revert changes for the okhttp and urlconnection
senders, which formerly used normal mockwebserver 3 and were
automatically migrated to v5 alpha.

Next, I had to polish formatting as it undid some of that, and also
fixed some cases where we were inconsistent. Then, I reviewed our base
pom and removed some special cases that were no longer needed. Finally,
I swept through and reduced visibility modifiers where possible.

Note: Both testcontainers and mockwebserver v3 have an implicit dep on
junit 4. This is ok and the same as zipkin. The main thing here is we
aren't explicitly using it anymore.

Signed-off-by: Adrian Cole <[email protected]>
  • Loading branch information
codefromthecrypt authored Dec 14, 2023
1 parent fc521af commit 963caa4
Show file tree
Hide file tree
Showing 31 changed files with 391 additions and 451 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@

@TestInstance(TestInstance.Lifecycle.PER_CLASS)
@Timeout(60)
public class ITActiveMQSender {
class ITActiveMQSender {
@RegisterExtension ActiveMQExtension activemq = new ActiveMQExtension();

@Test void checkPasses() {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2016-2020 The OpenZipkin Authors
* Copyright 2016-2023 The OpenZipkin Authors
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
Expand All @@ -13,7 +13,7 @@
*/
package zipkin2.reporter.amqp;

import org.junit.Test;
import org.junit.jupiter.api.Test;
import zipkin2.CheckResult;
import zipkin2.reporter.AsyncReporter;
import zipkin2.reporter.ClosedSenderException;
Expand All @@ -24,19 +24,19 @@
import static zipkin2.TestObjects.CLIENT_SPAN;
import static zipkin2.reporter.amqp.ITRabbitMQSender.send;

public class RabbitMQSenderTest {
class RabbitMQSenderTest {
// We can be pretty certain RabbitMQ isn't running on localhost port 80
RabbitMQSender sender = RabbitMQSender.newBuilder()
.connectionTimeout(100).addresses("localhost:80").build();

@Test public void checkFalseWhenRabbitMQIsDown() {
@Test void checkFalseWhenRabbitMQIsDown() {
CheckResult check = sender.check();
assertThat(check.ok()).isFalse();
assertThat(check.error())
.isInstanceOf(RuntimeException.class);
}

@Test public void illegalToSendWhenClosed() throws Exception {
@Test void illegalToSendWhenClosed() throws Exception {
sender.close();

assertThatThrownBy(() -> send(sender, CLIENT_SPAN, CLIENT_SPAN))
Expand All @@ -49,7 +49,7 @@ public class RabbitMQSenderTest {
* tools, care should be taken to ensure the toString() output is a reasonable length and does not
* contain sensitive information.
*/
@Test public void toStringContainsOnlySummaryInformation() {
@Test void toStringContainsOnlySummaryInformation() {
assertThat(sender).hasToString(
"RabbitMQSender{addresses=[localhost:80], queue=zipkin}"
);
Expand Down
5 changes: 0 additions & 5 deletions benchmarks/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -120,11 +120,6 @@
https://github.com/charithe/kafka-junit -->
<version>4.2.10</version>
</dependency>
<dependency>
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>${junit.version}</version>
</dependency>
<dependency>
<groupId>ch.qos.logback</groupId>
<artifactId>logback-classic</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,12 @@

import java.util.List;
import java.util.concurrent.TimeUnit;
import org.junit.Rule;
import org.junit.jupiter.api.extension.RegisterExtension;
import zipkin2.Span;
import zipkin2.junit5.ZipkinExtension;
import zipkin2.reporter.okhttp3.OkHttpSender;

public class BasicUsageTest_Async extends BasicUsageTest<AsyncZipkinSpanHandler> {
class BasicUsageTest_Async extends BasicUsageTest<AsyncZipkinSpanHandler> {
@RegisterExtension public ZipkinExtension zipkin = new ZipkinExtension();

OkHttpSender sender = OkHttpSender.create(zipkin.httpUrl() + "/api/v2/spans");
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2016-2020 The OpenZipkin Authors
* Copyright 2016-2023 The OpenZipkin Authors
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
Expand All @@ -16,7 +16,7 @@
import java.util.List;
import zipkin2.Span;

public class BasicUsageTest_Converting extends BasicUsageTest<ZipkinSpanHandler> {
class BasicUsageTest_Converting extends BasicUsageTest<ZipkinSpanHandler> {
@Override ZipkinSpanHandler zipkinSpanHandler(List<Span> spans) {
return (ZipkinSpanHandler) ZipkinSpanHandler.create(spans::add);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2016-2020 The OpenZipkin Authors
* Copyright 2016-2023 The OpenZipkin Authors
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
Expand All @@ -18,8 +18,8 @@
import brave.propagation.TraceContext;
import java.util.ArrayList;
import java.util.Objects;
import org.junit.Before;
import org.junit.Test;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import zipkin2.Annotation;
import zipkin2.Endpoint;
import zipkin2.Span;
Expand All @@ -32,7 +32,7 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.entry;

public class ConvertingSpanReporterTest {
class ConvertingSpanReporterTest {
static class ListSpanReporter extends ArrayList<Span> implements Reporter<Span> {
@Override public void report(Span span) {
add(span);
Expand All @@ -44,15 +44,15 @@ static class ListSpanReporter extends ArrayList<Span> implements Reporter<Span>
TraceContext context = TraceContext.newBuilder().traceId(1).spanId(2).sampled(true).build();
MutableSpan defaultSpan;

@Before public void init() {
@BeforeEach void init() {
defaultSpan = new MutableSpan(context, null);
defaultSpan.localServiceName("Aa");
defaultSpan.localIp("1.2.3.4");
defaultSpan.localPort(80);
spanReporter = new ConvertingSpanReporter(spans, Tags.ERROR);
}

@Test public void generateKindMap() {
@Test void generateKindMap() {
assertThat(ConvertingSpanReporter.generateKindMap()).containsExactly(
entry(CLIENT, Span.Kind.CLIENT),
entry(SERVER, Span.Kind.SERVER),
Expand All @@ -61,7 +61,7 @@ static class ListSpanReporter extends ArrayList<Span> implements Reporter<Span>
);
}

@Test public void equalsAndHashCode() {
@Test void equalsAndHashCode() {
assertThat(spanReporter)
.hasSameHashCodeAs(spans)
.isEqualTo(new ConvertingSpanReporter(spans, Tags.ERROR));
Expand All @@ -74,7 +74,7 @@ static class ListSpanReporter extends ArrayList<Span> implements Reporter<Span>
.isNotEqualTo(otherReporter.hashCode());
}

@Test public void convertsSampledSpan() {
@Test void convertsSampledSpan() {
MutableSpan span = new MutableSpan(context, null);
spanReporter.report(span);

Expand All @@ -86,7 +86,7 @@ static class ListSpanReporter extends ArrayList<Span> implements Reporter<Span>
);
}

@Test public void convertsDebugSpan() {
@Test void convertsDebugSpan() {
TraceContext context = TraceContext.newBuilder().traceId(1).spanId(2).debug(true).build();
MutableSpan span = new MutableSpan(context, null);
spanReporter.report(span);
Expand All @@ -100,7 +100,7 @@ static class ListSpanReporter extends ArrayList<Span> implements Reporter<Span>
);
}

@Test public void minimumDurationIsOne() {
@Test void minimumDurationIsOne() {
MutableSpan span = new MutableSpan(context, null);

span.startTimestamp(1L);
Expand All @@ -110,7 +110,7 @@ static class ListSpanReporter extends ArrayList<Span> implements Reporter<Span>
assertThat(spans.get(0).duration()).isEqualTo(1L);
}

@Test public void replacesTag() {
@Test void replacesTag() {
MutableSpan span = new MutableSpan(context, null);

span.tag("1", "1");
Expand All @@ -130,7 +130,7 @@ static class ListSpanReporter extends ArrayList<Span> implements Reporter<Span>

Throwable ERROR = new RuntimeException();

@Test public void backfillsErrorTag() {
@Test void backfillsErrorTag() {
MutableSpan span = new MutableSpan(context, null);

span.error(ERROR);
Expand All @@ -141,7 +141,7 @@ static class ListSpanReporter extends ArrayList<Span> implements Reporter<Span>
.containsOnly(entry("error", "RuntimeException"));
}

@Test public void doesntOverwriteErrorTag() {
@Test void doesntOverwriteErrorTag() {
MutableSpan span = new MutableSpan(context, null);

span.error(ERROR);
Expand All @@ -153,7 +153,7 @@ static class ListSpanReporter extends ArrayList<Span> implements Reporter<Span>
.containsOnly(entry("error", ""));
}

@Test public void addsAnnotations() {
@Test void addsAnnotations() {
MutableSpan span = new MutableSpan(context, null);

span.startTimestamp(1L);
Expand All @@ -166,19 +166,19 @@ static class ListSpanReporter extends ArrayList<Span> implements Reporter<Span>
.containsOnly(Annotation.create(2L, "foo"));
}

@Test public void finished_client() {
@Test void finished_client() {
finish(brave.Span.Kind.CLIENT, Span.Kind.CLIENT);
}

@Test public void finished_server() {
@Test void finished_server() {
finish(brave.Span.Kind.SERVER, Span.Kind.SERVER);
}

@Test public void finished_producer() {
@Test void finished_producer() {
finish(brave.Span.Kind.PRODUCER, Span.Kind.PRODUCER);
}

@Test public void finished_consumer() {
@Test void finished_consumer() {
finish(brave.Span.Kind.CONSUMER, Span.Kind.CONSUMER);
}

Expand All @@ -197,19 +197,19 @@ void finish(brave.Span.Kind braveKind, Span.Kind span2Kind) {
assertThat(zipkinSpan.kind()).isEqualTo(span2Kind);
}

@Test public void flushed_client() {
@Test void flushed_client() {
flush(brave.Span.Kind.CLIENT, Span.Kind.CLIENT);
}

@Test public void flushed_server() {
@Test void flushed_server() {
flush(brave.Span.Kind.SERVER, Span.Kind.SERVER);
}

@Test public void flushed_producer() {
@Test void flushed_producer() {
flush(brave.Span.Kind.PRODUCER, Span.Kind.PRODUCER);
}

@Test public void flushed_consumer() {
@Test void flushed_consumer() {
flush(brave.Span.Kind.CONSUMER, Span.Kind.CONSUMER);
}

Expand All @@ -228,7 +228,7 @@ void flush(brave.Span.Kind braveKind, Span.Kind span2Kind) {
assertThat(zipkinSpan.kind()).isEqualTo(span2Kind);
}

@Test public void remoteEndpoint() {
@Test void remoteEndpoint() {
MutableSpan span = new MutableSpan(context, null);

Endpoint endpoint = Endpoint.newBuilder()
Expand All @@ -250,7 +250,7 @@ void flush(brave.Span.Kind braveKind, Span.Kind span2Kind) {
}

// This prevents the server startTimestamp from overwriting the client one on the collector
@Test public void writeTo_sharedStatus() {
@Test void writeTo_sharedStatus() {
MutableSpan span = new MutableSpan(context, null);

span.setShared();
Expand All @@ -264,7 +264,7 @@ void flush(brave.Span.Kind braveKind, Span.Kind span2Kind) {
.isTrue();
}

@Test public void flushUnstartedNeitherSetsTimestampNorDuration() {
@Test void flushUnstartedNeitherSetsTimestampNorDuration() {
MutableSpan flushed = new MutableSpan(context, null);
flushed.finishTimestamp(0L);

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2016-2020 The OpenZipkin Authors
* Copyright 2016-2023 The OpenZipkin Authors
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except
* in compliance with the License. You may obtain a copy of the License at
Expand All @@ -19,15 +19,15 @@
import brave.propagation.TraceContext;
import java.util.ArrayList;
import java.util.Objects;
import org.junit.Before;
import org.junit.Test;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import zipkin2.reporter.Reporter;

import static brave.handler.SpanHandler.Cause.FINISHED;
import static brave.handler.SpanHandler.Cause.FLUSHED;
import static org.assertj.core.api.Assertions.assertThat;

public class ZipkinSpanHandlerTest {
class ZipkinSpanHandlerTest {
static class ListMutableSpanReporter extends ArrayList<MutableSpan>
implements Reporter<MutableSpan> {
@Override public void report(MutableSpan span) {
Expand All @@ -40,20 +40,20 @@ static class ListMutableSpanReporter extends ArrayList<MutableSpan>
TraceContext context = TraceContext.newBuilder().traceId(1).spanId(2).sampled(true).build();
MutableSpan defaultSpan;

@Before public void init() {
@BeforeEach void init() {
defaultSpan = new MutableSpan();
defaultSpan.localServiceName("Aa");
defaultSpan.localIp("1.2.3.4");
defaultSpan.localPort(80);
handler = new ZipkinSpanHandler(spans, Tags.ERROR, false);
}

@Test public void noopIsNoop() {
@Test void noopIsNoop() {
assertThat(ZipkinSpanHandler.create(Reporter.NOOP))
.isSameAs(SpanHandler.NOOP);
}

@Test public void equalsAndHashCode() {
@Test void equalsAndHashCode() {
assertThat(handler)
.hasSameHashCodeAs(spans)
.isEqualTo(new ZipkinSpanHandler(spans, Tags.ERROR, false));
Expand All @@ -66,41 +66,41 @@ static class ListMutableSpanReporter extends ArrayList<MutableSpan>
.isNotEqualTo(otherHandler.hashCode());
}

@Test public void reportsSampledSpan() {
@Test void reportsSampledSpan() {
MutableSpan span = new MutableSpan(context, null);
handler.end(context, span, FINISHED);

assertThat(spans.get(0)).isSameAs(span);
}

@Test public void reportsDebugSpan() {
@Test void reportsDebugSpan() {
TraceContext context = TraceContext.newBuilder().traceId(1).spanId(2).debug(true).build();
MutableSpan span = new MutableSpan(context, null);
handler.end(context, span, FINISHED);

assertThat(spans.get(0)).isSameAs(span);
}

@Test public void reportsFlushedSpan() {
@Test void reportsFlushedSpan() {
MutableSpan span = new MutableSpan(context, null);
handler.end(context, span, FLUSHED);

assertThat(spans.get(0)).isSameAs(span);
}

@Test public void doesnNotHandleAbandoned() {
@Test void doesnNotHandleAbandoned() {
assertThat(handler.handlesAbandoned()).isFalse();
}

@Test public void doesntReportUnsampledSpan() {
@Test void doesntReportUnsampledSpan() {
TraceContext context =
TraceContext.newBuilder().traceId(1).spanId(2).sampled(false).sampledLocal(true).build();
handler.end(context, new MutableSpan(context, null), FINISHED);

assertThat(spans).isEmpty();
}

@Test public void alwaysReportSpans_reportsUnsampledSpan() {
@Test void alwaysReportSpans_reportsUnsampledSpan() {
handler = new ZipkinSpanHandler(spans, Tags.ERROR, true);

TraceContext context =
Expand All @@ -110,7 +110,7 @@ static class ListMutableSpanReporter extends ArrayList<MutableSpan>
assertThat(spans).isNotEmpty();
}

@Test public void alwaysReportSpans_doesNotHandleAbandoned() {
@Test void alwaysReportSpans_doesNotHandleAbandoned() {
handler = new ZipkinSpanHandler(spans, Tags.ERROR, true);
assertThat(handler.handlesAbandoned()).isFalse();
}
Expand Down
Loading

0 comments on commit 963caa4

Please sign in to comment.