-
Couldn't load subscription status.
- Fork 41.6k
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)) | ||||||||||||
|
||||||||||||
| private final ApplicationContextRunner contextRunner = new ApplicationContextRunner() | |
| .withConfiguration(AutoConfigurations.of(DefaultEncodingConfiguration.class, OpenTelemetryConfiguration.class)); |
There are more occurrences like this, for example, as follows:
Lines 73 to 75 in 4babe2e
| private final ApplicationContextRunner contextRunner = new ApplicationContextRunner() | |
| .withConfiguration(AutoConfigurations.of(PulsarConfiguration.class)) | |
| .withBean(PulsarClient.class, () -> mock(PulsarClient.class)); |
I also thought that "User" in the withUserConfiguration() doesn't sound right for a part of the auto-configuration.
Are you suggesting that the mixed uses are okay, or that they need to be used with the withUserConfiguration() if it's not an @AutoConfiguration?
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.
AutoConfigurations.of does two things:
- It sorts all of the passed-in configurations based on their before and after attributes
- It orders the configurations so that they're processed before any user configurations
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 comment
The reason will be displayed to describe this comment to others. Learn more.
@wilkinsona Thanks for the explanation!
I reverted it in 00e2d1b.
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.