-
Notifications
You must be signed in to change notification settings - Fork 775
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
testing-common refactoring: replace direct AgentTestRunner usage with… #2134
testing-common refactoring: replace direct AgentTestRunner usage with… #2134
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you update https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/docs/contributing/writing-instrumentation.md#writing-instrumentation-tests and the section below to reflect these changes?
I forgot about these docs, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@@ -91,8 +91,8 @@ After writing a test or two, go back to the `library` package, make sure it has | |||
`testing` submodule and add a test that inherits from the abstract test class. You should implement | |||
the method to initialize the client using the library's mechanism to register interceptors, perhaps | |||
a method like `registerInterceptor` or wrapping the result of a library factory when delegating. The | |||
test should implement the `InstrumentationTestRunner` trait for common setup logic. If the tests | |||
pass, library instrumentation is working OK. | |||
test should implement the `LibraryTestTrait` trait for common setup logic. If the tests pass, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These names are much better!
|
||
static InstrumentationTestRunner instrumentationTestRunner | ||
static InMemorySpanExporter testWriter | ||
|
||
def setupSpec() { | ||
void runnerSetupSpec() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah this workaround is even nicer
6b94399
to
485db38
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice :)
ReactorCoreTest was getting a tracer from GlobalOpenTelemetry before LibraryTestTrait had a change to initialize the SDK
485db38
to
dac6492
Compare
… spock spec
Another part of #1644
I've replaced direct
AgentTestRunner
usages withAgentInstrumentationSpecification
, which is based on @anuraaga'sInstrumentationSpecification
& agent/library traits idea. The aim of this whole refactoring is to makeAgentTestRunner
(and other crucial test code) Java-only - and thus usable in non-spock tests.This PR is pretty big but it's mostly imports & renaming things.