+1 Great suggestion. -- Best Regards! crossoverJie
Zhangjian He <hezhangjia...@gmail.com> 于2025年8月26日周二 07:33写道: > +1 > > On Mon, Aug 25, 2025 at 11:38 PM Lari Hotari <lhot...@apache.org> wrote: > > > +1, Thanks for starting this discussion, Yunze. > > > > One good principle is to only test externally observable behaviors > > ("test behavior, not implementation"). > > Having the need to inspect internal state is a sign of bad design. It > > could also be a sign of bad test design where the test is checking > > that the implementation doesn't change. > > Such tests are very hard to maintain and hard to understand when the > > test isn't about testing behaviors, but instead tests that the > > implementation has been done in a specific way. > > Obviously there are exceptions, and sometimes checking internal state > > could be useful for confirming assumptions in tests. > > We should break down logic more into internal abstractions (internal > > APIs) in separate classes and unit test them in isolation. Internal > > abstractions can define desired outcomes and behavior, and it remains > > an implementation detail how this is achieved. > > Tests shouldn't test implementation details. Instead, we should prefer > > testing through proper behavioral APIs over inspecting internal state > > and injecting internal state for testing purposes. > > It should be sufficient to test externally observable behaviors in > > most cases. This applies also to internal abstractions. If the > > observable behavior is only available through some internal state, > > that's a sign that something is not right in the design. > > > > > > -Lari > > > > On Mon, 25 Aug 2025 at 16:39, Yunze Xu <x...@apache.org> wrote: > > > > > > 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 > > > > > -- > Best Regards, > Zhangjian He > Twitter: hezhangjian > WeChat: hezhangjian97 >