You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pulsar.apache.org by Yunze Xu <xy...@apache.org> on 2025/08/25 13:39:27 UTC
[DISCUSS] Stop using reflection to access private fields in tests
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
Re: [DISCUSS] Stop using reflection to access private fields in tests
Posted by Jie crossover <cr...@gmail.com>.
+1
Great suggestion.
--
Best Regards！
crossoverJie
Zhangjian He <he...@gmail.com> 于2025年8月26日周二 07:33写道：
> +1
>
> On Mon, Aug 25, 2025 at 11:38 PM Lari Hotari <lh...@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 <xy...@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
>
Re: [DISCUSS] Stop using reflection to access private fields in tests
Posted by Zhangjian He <he...@gmail.com>.
+1
On Mon, Aug 25, 2025 at 11:38 PM Lari Hotari <lh...@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 <xy...@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
Re: [DISCUSS] Stop using reflection to access private fields in tests
Posted by Lari Hotari <lh...@apache.org>.
+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 <xy...@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
Re: [DISCUSS] Stop using reflection to access private fields in tests
Posted by Tao Jiuming <da...@apache.org>.
+1
xiangying meng <xi...@apache.org>于2025年8月26日 周二10:39写道：
> +1
>
> BR,
> Xiangying
>
> On Tue, Aug 26, 2025 at 10:13 AM Ruimin MA <ma...@gmail.com> wrote:
> >
> > thanks for sharing this. It's really helpful and good to know that!
> >
> >
> > Yunze Xu <xy...@apache.org>于2025年8月25日 周一21:39写道：
> >
> > > 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
> > >
>
Re: [DISCUSS] Stop using reflection to access private fields in tests
Posted by xiangying meng <xi...@apache.org>.
+1
BR,
Xiangying
On Tue, Aug 26, 2025 at 10:13 AM Ruimin MA <ma...@gmail.com> wrote:
>
> thanks for sharing this. It's really helpful and good to know that!
>
>
> Yunze Xu <xy...@apache.org>于2025年8月25日 周一21:39写道：
>
> > 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
> >
Re: [DISCUSS] Stop using reflection to access private fields in tests
Posted by Ruimin MA <ma...@gmail.com>.
thanks for sharing this. It's really helpful and good to know that!
Yunze Xu <xy...@apache.org>于2025年8月25日 周一21:39写道：
> 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
>