Skip to content

Commit b4df3c6

Browse files
authored
[improve][ci] Document avoiding reflection in tests in Copilot review instructions (#25636)
1 parent 4f73b43 commit b4df3c6

1 file changed

Lines changed: 46 additions & 0 deletions

File tree

.github/copilot-instructions.md

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,50 @@ Flag any change that may break compatibility.
249249
* Assertions should prefer using AssertJ library with descriptions over using TestNG assertions
250250
* Awaitility should be used to handle assertions with asynchronous conditions together with AssertJ
251251

252+
## Avoid Reflection in Tests
253+
254+
Do **not** use reflection to access private fields or methods from tests. This includes
255+
`WhiteboxImpl.getInternalState`, `WhiteboxImpl.setInternalState`, `Field.setAccessible(true)`,
256+
`Method.setAccessible(true)`, and similar reflection helpers.
257+
258+
Reflection-based test access is discouraged because it:
259+
260+
* breaks during refactoring without any compile-time signal — renaming or retyping a field
261+
silently invalidates the test
262+
* produces verbose, brittle, and hard-to-read test code
263+
* couples tests to implementation details that should be free to change
264+
265+
Instead, expose what tests legitimately need through **package-private** methods (or fields
266+
where appropriate) and annotate them with `@VisibleForTesting`:
267+
268+
```java
269+
@VisibleForTesting
270+
Map<String, Subscription> getSubscriptions() {
271+
return subscriptions;
272+
}
273+
```
274+
275+
The test then accesses state through a normal, statically-typed call:
276+
277+
```java
278+
var subscriptions = persistentTopic.getSubscriptions();
279+
```
280+
281+
instead of:
282+
283+
```java
284+
ConcurrentOpenHashMap<String, PersistentSubscription> subscriptions =
285+
WhiteboxImpl.getInternalState(persistentTopic, "subscriptions");
286+
```
287+
288+
Place the test in the same package as the class under test so package-private visibility is
289+
sufficient — no need to widen visibility to `public` for testing.
290+
291+
When reviewing PRs, flag any new use of reflection in tests and suggest a `@VisibleForTesting`
292+
package-private accessor instead. See the dev@ thread
293+
[Stop using reflection to access private fields in tests](https://lists.apache.org/thread/7gr04sqmzyttx4ln6ydtp3qv0xgo1o6m)
294+
for the full rationale.
295+
252296
# Testing Expectations
253297

254298
Every feature or bug fix should include tests.
@@ -278,5 +322,7 @@ When reviewing a pull request, Copilot should:
278322
* ensure logging follows project guidelines
279323
* verify backward compatibility
280324
* suggest missing tests when appropriate
325+
* flag reflection-based access to private fields or methods in tests (e.g. `WhiteboxImpl`,
326+
`setAccessible(true)`) and recommend a package-private `@VisibleForTesting` accessor instead
281327

282328
Focus feedback on correctness, reliability, and maintainability.

0 commit comments

Comments
 (0)