Skip to content

[improve][ci] Document avoiding reflection in tests in Copilot review instructions#25636

Merged
merlimat merged 1 commit intoapache:masterfrom
lhotari:lh-update-copilot-ins-no-reflection
Apr 30, 2026
Merged

[improve][ci] Document avoiding reflection in tests in Copilot review instructions#25636
merlimat merged 1 commit intoapache:masterfrom
lhotari:lh-update-copilot-ins-no-reflection

Conversation

@lhotari
Copy link
Copy Markdown
Member

@lhotari lhotari commented Apr 30, 2026

Motivation

The Pulsar codebase has accumulated many tests that use reflection (WhiteboxImpl.getInternalState, setAccessible(true), etc.) to read or mutate private fields of the class under test. This pattern was discussed on the dev@ mailing list:

[DISCUSS] Stop using reflection to access private fields in tests

Reflection-based test access:

  • breaks during refactoring without any compile-time signal — renaming or retyping a field silently invalidates the test
  • produces verbose, brittle, and hard-to-read test code
  • couples tests to implementation details that should be free to change

The discussion concluded that the preferred approach is to expose what tests legitimately need through package-private methods annotated with @VisibleForTesting, with the test placed in the same package as the class under test.

This PR teaches the Copilot PR-review bot to flag reflection-based test access in new code, so the project guidance is enforced consistently in reviews going forward.

Modifications

.github/copilot-instructions.md:

  • Added an Avoid Reflection in Tests subsection under Testing Guidelines with the rationale, the preferred @VisibleForTesting package-private pattern, before/after examples (WhiteboxImpl.getInternalState vs. a typed getter), and a link to the dev@ thread.
  • Added a matching item to the Pull Request Review Guidance checklist so reviewers proactively flag new reflection use.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage. It only modifies Copilot reviewer guidance in .github/copilot-instructions.md; there is no production or test code change.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

… instructions

Add guidance to Copilot review instructions that prohibits using
reflection (e.g. WhiteboxImpl, setAccessible) to access private fields
or methods from tests, and recommends exposing package-private
accessors annotated with @VisibleForTesting instead.

Based on the dev@ thread:
https://lists.apache.org/thread/7gr04sqmzyttx4ln6ydtp3qv0xgo1o6m
@lhotari lhotari changed the title [improve][CI] Document avoiding reflection in tests in Copilot review instructions [improve][ci] Document avoiding reflection in tests in Copilot review instructions Apr 30, 2026
@merlimat merlimat merged commit b4df3c6 into apache:master Apr 30, 2026
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants