-
Notifications
You must be signed in to change notification settings - Fork 40.8k
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
Add tests to verify that every config setting for each auto-configured meter registry is exposed as a configuration property #33743
Conversation
@msobeck Please sign the Contributor License Agreement! Click here to manually synchronize the status of this Pull Request. See the FAQ for frequently asked questions. |
@msobeck Thank you for signing the Contributor License Agreement! |
574a23a
to
78b8d0d
Compare
@wilkinsona thanks for the review! Is there anything left in this PR? Otherwise I would suggest to close this one, since the missing tests were added in ff04f00. |
Yeah, there's quite a bit left. ff04f00 didn't include any of the changes to verify that each config setting has a property. That's what we'll merge in this PR. I'm in the process of polishing those changes: https://github.com/wilkinsona/spring-boot/tree/gh-33743. |
|
||
} | ||
|
||
private static final List<Class<?>> classesForPropertiesList = List.of(Boolean.class, Byte.class, Character.class, |
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 we add Enum
too?
We have quite a few use-cases, e.g.: HistogramFlavor
for PrometheusConfig
or DynatraceApiVersion
for DynatraceConfig
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 for taking a look, @jonatan-ivanov. I've removed the allow-list approach in my polishing. We now consider all default methods with no parameters, except those that are deprecated or that return Validated. The code that does that is here.
I really like the idea of having verifications for this, thank you! |
* spring-projectsgh-33743: Polish "Test Micrometer config to property exposure" Test Micrometer config to property exposure Closes spring-projectsgh-33743
Thanks very much, @msobeck. |
Closes #30901.
I added the missing
PropertiesConfigAdapterTests
and enhanced them, so that newly addedfields to micrometer configs, that are not settable via properties, will let the test fail. (if you don't specifically exclude them)
Not overridden default methods in adapters are the most common cause of forgotten field exposure in this scenario, because they do not for force an override in the adapter. (see #30898)
TestConfigsToPropertiesExposure
will check for not overridden config default methods in the adapter class.This can be an indicator for micrometer config fields, that have been forgotten to expose via properties.
They already have found some not exposed config fields f.e. for
AtlasConfig
:I declared them as excluded, because i don't know if they are intentionally not exposed.
Someone could look at this in another task. This PR only shows the current state.
I am aware that these tests are just a rough help against human forgetfulness and can be reverted if they don't fit in.