Hi all, I'd like to bring the open discussion for the coding style about whether to use reflection in tests. Today when I reviewed a PR, I left a comment here [1] because I noticed reflection was used again in tests.
Reflection makes refactoring really painful and could kill the enthusiasm to refactor the existing bad quality code. [2] is an example when I tried to replace ConcurrentOpenHashMap with ConcurrentHashMap. You can find how many `WhiteboxImpl` references are in that PR. The painful point is that if a field's type is changed and this field was accessed via reflection in tests, it could not be detected during compilation. I can feel the pain because I've contributed many refactoring PRs to improve the code, it really has annoyed me many times when I found a new failed test due to not being exposed by reflection. Even regardless of the refactoring, using reflection in tests is a bad practice. Pulsar has adopted Java 17 for years, though many people still don't like `var`. You can compare the following two sentences: ```java ConcurrentOpenHashMap<String, PersistentSubscription> subscriptions = WhiteboxImpl.getInternalState(persistentTopic, "subscriptions"); ``` ``` var subscriptions = persistentTopic.getSubscriptions(); ``` The 1st one is really long and hard to refactor, while the 2nd one is short and scalable so that it works even if `getSubscriptions` returns a different map type in future. I know the debate about anonymous typing widely exists. But anyway, nearly all modern languages have adopted this solution, e.g. `var`, `let`, `auto`, `val`. I don't mean to say using `var` is always better than writing the full type name. But anyway, you can write the type name if you want, like: ``` Map<String, Subscription> subscriptions = persistentTopic.getSubscriptions(); ``` It's still more short and scalable than the 1st one. The only disadvantage of exposing a field's visibility for tests is that it breaks the encapsulation. But we really don't need much encapsulation for non-public APIs. And we have the `@VisibleForTesting` annotation. Getters and setters in Java have been criticized by many users of other languages. Keeping a field private and using reflection to access it really looks like a joke. Therefore, I hope when you're reviewing PRs, please prevent reflections in tests as much as possible. [1] https://github.com/apache/pulsar/pull/24658#discussion_r2298080559 [2] https://github.com/apache/pulsar/pull/23320/files [3] https://stackoverflow.com/questions/2811141/is-it-bad-practice-to-use-reflection-in-unit-testing Thanks, Yunze
