capistrant commented on code in PR #12504:
URL: https://github.com/apache/druid/pull/12504#discussion_r892382695
##########
server/src/test/java/org/apache/druid/server/http/CoordinatorDynamicConfigTest.java:
##########
@@ -89,6 +89,7 @@ public void testSerde() throws Exception
9,
false,
false,
+ Integer.MAX_VALUE,
Review Comment:
I am starting to wonder, in general, what the value of this test class is
providing. I see value in testing some things:
* that invalid values blow up serde of `CoordinatorDynamicConfig` where
expected.
* testing `CoordinatorDynamicConfig#isKillUnusedSegmentsInAllDataSources`
* testing handling of nullable fields for `CoordinatorDynamicConfig`
* testing
`CoordinatorDynamicConfig.Builder#build(CoordinatorDynamicConfig)`
But as is, the current class is a fairly unorganized set of tests. I even
see that some new dynamic config values are completely untested.
`useBatchedSegmentSampler` is an example of that. Since it is a primitive with
standard system defaults used when missing in payload being deserialized, maybe
it makes sense not to test. But based on the standard in the tests today,
omitting it seems like a break in the existing pattern.
In my follow on commit responding to review, I will re-organize the tests
into what I see value in. This will be opinionated and should be discussed
further at that time. Perhaps there is some standard testing patterns for a
class like this out there that I am unaware of and could be followed instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
