+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
