-
Notifications
You must be signed in to change notification settings - Fork 41.5k
Polish #46505
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
Polish #46505
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -44,11 +44,12 @@ class ZipkinConfigurationsOpenTelemetryConfigurationTests { | |||||||||||
|
||||||||||||
@Test | ||||||||||||
void backsOffWithoutEncoding() { | ||||||||||||
new ApplicationContextRunner().withUserConfiguration(OpenTelemetryConfiguration.class).run((context) -> { | ||||||||||||
assertThat(context).hasNotFailed(); | ||||||||||||
assertThat(context).doesNotHaveBean(ZipkinSpanExporter.class); | ||||||||||||
assertThat(context).doesNotHaveBean(BytesEncoder.class); | ||||||||||||
}); | ||||||||||||
new ApplicationContextRunner().withConfiguration(AutoConfigurations.of(OpenTelemetryConfiguration.class)) | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @wilkinsona I don't know exactly what it does behind the scenes, but I saw the following code in the same class, so I thought that a part of the auto-configuration also uses Lines 42 to 43 in 4babe2e
There are more occurrences like this, for example, as follows: Lines 73 to 75 in 4babe2e
I also thought that "User" in the Are you suggesting that the mixed uses are okay, or that they need to be used with the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In this case, the configuration isn't an auto-configuration so there are no before and after attributes to consider and there's only of them so there's nothing to sort. There's also only one configuration class in the test so the order in which they're processed doesn't matter either. We're pretty inconsistent about this across our tests. What was there before is fine and what you've proposed is also fine. Given this, I'd prefer to leave things as they were. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @wilkinsona Thanks for the explanation! I reverted it in 00e2d1b. |
||||||||||||
.run((context) -> { | ||||||||||||
assertThat(context).hasNotFailed(); | ||||||||||||
assertThat(context).doesNotHaveBean(ZipkinSpanExporter.class); | ||||||||||||
assertThat(context).doesNotHaveBean(BytesEncoder.class); | ||||||||||||
}); | ||||||||||||
} | ||||||||||||
|
||||||||||||
@Test | ||||||||||||
|
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.
I think "rely" is correct here.
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.
@wilkinsona Thanks for the feedback!
I reverted it in 32c9bbe.