+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
