apache / apache/jmeter

Split `TestBean` into its GUI-generation and field-storage halves

Open
#6,728 1 comment 1 reaction 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.