Split `TestBean` into its GUI-generation and field-storage halves
- Dominant language
- Java
- Stars
- 9.5k
- Forks
- 2.3k
- Avg merge
- 1d 22h
- Merged PRs (30d)
- 5
Description
## Summary
`TestBean` is a marker interface that means two unrelated things at once. Elements can't opt into one without the other, which is what turned a simple bug report (#6451) into an open-ended design question in #6726. This issue proposes splitting it, and asks for a decision on how existing elements opt out of the storage half.
## Background
Implementing `TestBean` currently buys an element two separate things:
1. **A generated GUI.** Write a `BeanInfo` and `TestBeanGUI` builds the editor by introspection, with no Swing code in the element.
2. **Field-backed storage with a sync step.** `TestBeanHelper.prepare()` copies the property map into the element's setters, so the element's plain Java fields hold the values during a run.
The second is the source of a recurring surprise: the fields are only populated after `prepare()`, so every TestBean getter returns `null` (or a field initializer) on a tree that was just loaded from JMX. #6451 hit this with `CSVDataSet.getFilename()`. It's working as designed, but the design is a trap, and it applies to all 32 elements that implement `TestBean` — JSR223 elements, `ConstantThroughputTimer`, `DataSourceElement`, and the rest.
The two jobs have nothing to do with each other. An element that stores its configuration in the property map still wants a generated GUI; it just doesn't want the field sync.
## The split is already there in the code
Every place the framework tests for `TestBean` uses it for exactly one of the two jobs, and no call site needs both:
**GUI generation (5)**
- `TestBeanGUI:159` — guard that the class is a TestBean
- `GuiPackage:368` — instantiate `TestBeanGUI` for the element
- `MenuFactory:138`, `MenuFactory:209` — menu registration
- `JMeterTreeNode:145` — icon and label resolution
**Field-backed storage (3)**
- `TestBeanHelper:129` — entry guard for `prepare()`
- `TestBeanHelper:180` — recursion into nested `TestElementProperty`
- `StandardJMeterEngine:243` — prepare test listeners
## Proposal
Split the marker in two and have `TestBean` extend both. Names are open to bikeshedding:
```java
public interface BeanInfoGui {} // "generate my GUI from my BeanInfo"
public interface FieldBackedProperties {} // "copy my properties into my setters before use"
public interface TestBean extends BeanInfoGui, FieldBackedProperties {}
```
Then narrow the eight call sites above to whichever half they actually mean.
## This is a no-op for existing code
Adding superinterfaces to an existing interface is binary compatible, and because `TestBean` extends both, **every `instanceof` result stays identical**:
- `el instanceof TestBean` at `TestBeanHelper:129` becomes `el instanceof FieldBackedProperties`. Any element implementing `TestBean` inherits it, so the result is unchanged.
- `TestBean.class.isAssignableFrom(c)` at `GuiPackage:368` becomes `BeanInfoGui.class.isAssignableFrom(c)`. Same.
All 32 in-tree elements and every third-party plugin keep working with no source change. Both interfaces must stay method-free markers for this to hold.
So the split can land on its own merits, independently of anything else discussed here.
## What it enables
New elements implement only `BeanInfoGui`: they get the generated GUI, store their configuration in the property map, and their getters are valid at any point in the lifecycle. No marker, no opt-out, no `prepare()` on the sampling path.
That also unblocks the newer property editors (`JBooleanPropertyEditor` and friends) and the modified-value gutter for these elements, since those bind to schema property descriptors rather than to bean fields.
## The unresolved part: existing elements can't un-implement
An element like `CSVDataSet` has to keep implementing `TestBean` for compatibility — user code may cast to it or test for it. Since `TestBean extends FieldBackedProperties`, it inherits the storage half and Java gives us no way to drop it. So migrating an existing element to property-backed storage still needs an explicit opt-out. Options:
**A. A negative marker.** `interface NotFieldBacked {}`, checked alongside the positive one in `TestBeanHelper.prepare()`. Visible in the element's declaration, and its javadoc can scope it honestly as a legacy-compatibility wart that new elements should never need. Cost: a double negative in the type hierarchy.
**B. A BeanInfo attribute.** `beanDescriptor.setValue("propertyBacked", TRUE)`, read once inside the existing `GOOD_PROPS` `ClassValue` in `TestBeanHelper`, so zero per-call cost. The "cannot un-implement" problem disappears because this is metadata rather than a type, and the decision sits with the element's other bean metadata. Cost: stringly-typed and less visible when reading the element class.
**C. Neither** — accept that existing elements stay field-backed forever, and property-backed storage is only for new elements. Cheapest, but leaves the #6451 trap in place for all 32.
I lean toward B, precisely because "cannot un-implement an interface" is a permanent Java constraint and metadata sidesteps it entirely. I don't feel strongly.
## Decisions requested
These are independent; the first can land without settling the second.
1. Do we split `TestBean` as described?
2. If we do, which opt-out mechanism for existing elements — A, B, or C?
And if the answer to 2 is A or B, two semantics questions need settling once, project-wide, rather than being rediscovered per element:
- Should a getter return `null` or the schema default for an unset property? Property-backed getters return the default, which differs from today's field behavior before `prepare()`.
- Function properties: today `prepare()` freezes `${...}` into a field at compile time; property-backed access re-evaluates per read, using `FunctionProperty`'s per-iteration cache. The latter looks more correct, but it is an observable change.
## Origin
Came out of #6451 (reported as a `CSVDataSet.getFilename()` bug, closed as works-as-designed) and #6726, where @e345ee implemented property-backed storage for `CSVDataSet` with a `PropertyBackedTestBean` marker. That PR is a working reference implementation of the migration; it's on hold pending this decision.
Contributor guide
Research direction
Start by reading the listed TestBean checks in TestBeanGUI, GuiPackage, MenuFactory, JMeterTreeNode, TestBeanHelper, and StandardJMeterEngine, then review the property-backed CSVDataSet work in #6726. The change is complete only after the project decides the two marker interfaces and the opt-out semantics, updates all eight call sites, and preserves compatibility for existing elements.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- testing-qa
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100