Convert HttpClientTest to JUnit #3652
Conversation
…nstrumentation into java-httpclient-test
anuraaga
requested review from
iNikem,
jkwatson,
laurit,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
There was a problem hiding this comment.
While there are some issues with verbosity of Java assertions, I think we do have the goal of presenting our test framework, the base classes, for use from Java tests, especially since we can expect external Java instrumentation to need them. Hopefully it's ok.
👍
I'd like to continue to improve on this in the sdk-testing package (and am glad to have fixed a few gaps through the process of migrating this test).
all of these improvements in sdk-testing is great 👍
| SemanticAttributes.HTTP_USER_AGENT | ||
| ] | ||
| } | ||
|
|
||
| def "basic #method request #url"() { |
There was a problem hiding this comment.
| def "basic #method request #url"() { | |
| def "basic GET request #path"() { |
|
Thanks @trask. @mateuszrzeszutek would be great to get your review too since this touches the test runner stuff :) |
| /** Return a list of all captured traces, where each trace is a sorted list of spans. */ | ||
| default List<List<SpanData>> traces() { | ||
| return TelemetryDataUtil.groupTraces(getExportedSpans()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Should we make this method @Deprecated? It won't be used after sdk-testing 1.5.0 releases
There was a problem hiding this comment.
I lean towards not bothering for these alpha modules - I figure there is lots of similar cruft that could be marked deprecated but we'll just remove :P
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package io.opentelemetry.instrumentation.testing.junit; |
There was a problem hiding this comment.
Nit: WDYT about putting all http test related classes into ...testing.junit.http package?
| import io.opentelemetry.instrumentation.testing.LibraryTestRunner; | ||
| import org.junit.jupiter.api.extension.ExtensionContext; | ||
|
|
||
| public final class HttpClientLibraryInstrumentationExtension extends InstrumentationExtension { |
There was a problem hiding this comment.
HttpClientLibraryInstrumentationExtension is very similar to HttpClientAgentInstrumentationExtension - I'm wondering if we can have sth like this instead:
public static InstrumentationExtension agent() {
return new HttpClientInstrumentationExtension(AgentTestRunner.instance());
}
public static InstrumentationExtension library() {
return new HttpClientInstrumentationExtension(LibraryTestRunner.instance());
}The same could be done for AgentInstrumentationExtension/LibraryInstrumentationExtension. WDYT?
There was a problem hiding this comment.
Nice! Will do the same for the current extensions in a different PR
| }); | ||
| } | ||
|
|
||
| testing.waitAndAssertTraces(assertions.toArray(new Consumer[0])); |
There was a problem hiding this comment.
Should we add a waitAndAssertTraces(Collection< TraceAssert>) variant?
| return sendRequest(request, method, uri, headers); | ||
| } | ||
|
|
||
| // ideally private, but then groovy closures in this class cannot find them |
There was a problem hiding this comment.
Comment no longer applies?
| // ideally private, but then groovy closures in this class cannot find them |
| where: | ||
| method = "GET" | ||
| expect: | ||
| junitTest.basicRequestWith2Redirects() | ||
| } | ||
|
|
||
| def "basic #method request with circular redirects"() { |
There was a problem hiding this comment.
| def "basic #method request with circular redirects"() { | |
| def "basic request with circular redirects"() { |
| assumeTrue(testRedirects()) | ||
| assumeTrue(testCircularRedirects()) | ||
| expect: | ||
| junitTest.circularRedirects() | ||
| } | ||
|
|
||
| def "redirect #method to secured endpoint copies auth header"() { |
There was a problem hiding this comment.
| def "redirect #method to secured endpoint copies auth header"() { | |
| def "redirect to secured endpoint copies auth header"() { |
| } | ||
|
|
||
| def "high concurrency test with callback"() { | ||
| setup: | ||
| assumeTrue(testCausality()) |
There was a problem hiding this comment.
Is it necessary to duplicate assumptions? The JUnit delegate test already calls them.
There was a problem hiding this comment.
Unfortunately, the jupiter assumption exception is not recognized by junit4 / spock :(
| //which is guaranteed to use the same connection for all requests | ||
| SingleConnection createSingleConnection(String host, int port) { | ||
| return null | ||
| // ideally private, but then groovy closures in this class cannot find them |
There was a problem hiding this comment.
Is this comment still valid? This class doesn't call doRequest(), tests extending this one call it.
There was a problem hiding this comment.
Think so - going to look in a followup on whether this method can be removed
| return exception | ||
| } | ||
|
|
||
| // parent span must be cast otherwise it breaks debugging classloading (junit loads it early) |
There was a problem hiding this comment.
Is this comment still valid? Now that we have a separate testing java agent there should be no problem with loading that stuff early (probably...)
While there are some issues with verbosity of Java assertions, I think we do have the goal of presenting our test framework, the base classes, for use from Java tests, especially since we can expect external Java instrumentation to need them. Hopefully it's ok.
The Groovy API is mostly preserved, but the big difference is the assertion failures. The assertions print for each layer in the stack (traces, trace, span, attributes for example) and tend to have the most important information at the bottom. This isn't so terrible except that if there is an exception event in the data, then a huge exception stack trace gets printed multiple times. I'd like to continue to improve on this in the sdk-testing package (and am glad to have fixed a few gaps through the process of migrating this test). On the bright side, at least for me, an assertion failure like "NET_PEER_PORT expected to be <443> but was <-1>" is much easier to understand how to fix than the Spock assertions that dump a lot of information and have dashes trying to point out what is wrong.
A big cleanup I'll follow up with is replacing all the Test subclass method knobs with a single
HttpClientOptions/ Builder class which will reduce the forwarding, and also improve the readability of the subclasses I think.I'll also look through the TestRunner code a bit, I moved some code around in this PR but think there is still a bit more shuffling to make it consistent (some logic that could have been in runner was duplicated in the JUnit and Spock calling classes).