Add standalone @DataProvider methods#2379
Draft
leonard84 wants to merge 7 commits into
Draft
Conversation
Behavior-preserving refactor preparing for standalone @dataProvider methods: - Introduce IBaseDataIterator<T> as the generic base of IDataIterator (which stays a non-generic Object[] specialization, so all existing references keep compiling unchanged) and move the shared members (estimated count, data variable names, where-variable values, NO_WHERE_VARIABLE_VALUES) to the base. - Widen the estimate check in BaseDataIterator.estimateNumIterations from IDataIterator to IBaseDataIterator. - Introduce IDataIterationContext abstracting the iterator chain's coupling to SpockExecutionContext, IRunSupervisor and FeatureInfo (error sink, generated methods, providers, invocation targets, filter-logging config), with FeatureDataIterationContext as the execution-context-backed adapter used by the feature path. - Rename FeatureDataProviderIterator to DataProviderIterators and expose the chain construction as createDataIterator(context) so a standalone context can reuse the identical chain.
Behavior-preserving refactor preparing for standalone @dataProvider methods: - WhereBlockRewriter.rewrite() now runs an explicit parse step that records per-provider DataProviderArtifacts (provider expression, the data variables it supplies, previous data table variables) followed by the unchanged feature emission (provider/processor/wherevars/ multiplications/filter methods plus feature parameter handling). - Per-provider state is captured eagerly while parsing because it depends on parse progress (e.g. derived variables declared after a provider must not leak into its DataProviderMetadata). - New WhereBlockRewriter.parse(...) entry point exposes the parsed artifacts without emitting methods, for the standalone path that emits them as inline closures instead; instance-field access checking is optional there because plain (non-spec) classes allow instance field access.
A method annotated with spock.lang.DataProvider has its body parsed as where-block content (where-block variables, data tables, data pipes, derived columns, combined: multiplications, filter: blocks) and returns a fresh, lazy Iterator<Tuple> of rows that any feature can consume via the existing multi-variable data pipe ([a, b] << pairs()). Compiler side: - DataProviderMethodTransformation, a local SEMANTIC_ANALYSIS transform attached to the annotation, so it fires on any class, spec or not. SpecParser skips @dataProvider methods so the global SpockTransform hands the local transform the identical raw body on both paths. - DataProviderMethodRewriter splits the body at the filter: label, parses it via the shared WhereBlockRewriter builder, and replaces the body with a StandaloneDataProviders.dataIterator(...) call whose arguments are the parsed artifacts emitted as inline closures, so method parameters and 'this' are captured naturally. It then reruns a VariableScopeVisitor to rebuild the new closures' scopes. - Method parameters are body-wide inputs (like final where-block variables), kept verbatim in the signature, never columns, and never auto-closed; name collisions with data variables are compile errors. - def/Object return types get a synthesized Iterator<TupleN> (resolved dynamically, degrading to Iterator<Tuple> for arities the compiling Groovy version does not ship); explicit return types are validated. - @CompileStatic is rejected with a clear error; the instance-field access rule applies only inside specifications. - @StandaloneDataProviderMetadata records column names and line. Runtime side: - StandaloneDataProviders builds the regular data iterator chain via the standalone IDataIterationContext (closure-backed MethodInfos, throw-on-error, no supervisor) and decorates it as an IStandaloneDataIterator: rows map to arity-specific tuples (Tuple1.. Tuple16 resolved reflectively, generic Tuple fallback on Groovy 2.5 for arity 10+), close() is idempotent, and exhaustion closes eagerly so AutoCloseable where-block variables are released even when the iterator is consumed indirectly and discarded. - The consuming feature picks up the estimated iteration count through the IBaseDataIterator estimate widening from the engine refactor.
- Add a 'Standalone Data Provider Methods' section to the data-driven testing docs with runnable samples in DataSpec (basic data table provider consumed via [a, b] << provider(), and a parameterized static provider with a where-block local variable, derived column and filter block), plus a release notes entry. - Extend the test suite with the estimated-iteration-count pickup by a consuming feature (known and unknown counts), where-block variables visible to pipes and the filter block, and the '||' table separator.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces standalone ChangesStandalone
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@spock-core/src/main/java/org/spockframework/compiler/DataProviderMethodRewriter.java`:
- Around line 203-205: handleReturnType() currently returns for any
generics[0].isWildcard(), which incorrectly allows bounded wildcards; change the
early-return condition so it only returns for an unbounded wildcard
(Iterator<?>). Concretely, in DataProviderMethodRewriter.handleReturnType(),
replace the generics[0].isWildcard() check with a check that ensures the
wildcard has no explicit bounds (use generics[0].getUpperBounds() and
generics[0].getLowerBound() to detect an unbounded wildcard — e.g. upper bound
is Object or no meaningful upper bound and lower bound is null). If the wildcard
is bounded (has a non-Object upper bound or a non-null lower bound), do NOT
return early; instead report a validation error (use the class’s existing error
reporting helper used elsewhere in this rewriter, e.g., addError(...) or the
same mechanism used for other return-type errors) so Iterator<? extends
...>/Iterator<? super ...> do not bypass Tuple/arity checks.
In
`@spock-core/src/main/java/org/spockframework/runtime/DataIteratorFactory.java`:
- Around line 516-523: The code calls Arrays.stream(...) on the result of
invokeRaw(...) without checking for null; change the block in
DataIteratorFactory so you first capture the invokeRaw result into a temporary
DataVariableMultiplication[] (e.g. DataVariableMultiplication[] arr =
(DataVariableMultiplication[]) invokeRaw(null,
dataVariableMultiplicationsMethod)), then if arr == null return null immediately
(since invokeRaw already recorded the error), otherwise set
dataVariableMultiplications = Arrays.stream(arr).iterator() and
nextDataVariableMultiplication = dataVariableMultiplications.next(); keep the
existing try/catch around the invokeRaw/streaming to handle any other Throwable
and call context.error(...) there as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef6cdfc2-0227-4c87-a1ca-00e00ce405d3
📒 Files selected for processing (20)
docs/data_driven_testing.adocdocs/release_notes.adocspock-core/src/main/java/org/spockframework/compiler/AstNodeCache.javaspock-core/src/main/java/org/spockframework/compiler/DataProviderMethodRewriter.javaspock-core/src/main/java/org/spockframework/compiler/DataProviderMethodTransformation.javaspock-core/src/main/java/org/spockframework/compiler/SpecParser.javaspock-core/src/main/java/org/spockframework/compiler/WhereBlockRewriter.javaspock-core/src/main/java/org/spockframework/runtime/DataIteratorFactory.javaspock-core/src/main/java/org/spockframework/runtime/FeatureDataIterationContext.javaspock-core/src/main/java/org/spockframework/runtime/IBaseDataIterator.javaspock-core/src/main/java/org/spockframework/runtime/IDataIterationContext.javaspock-core/src/main/java/org/spockframework/runtime/IDataIterator.javaspock-core/src/main/java/org/spockframework/runtime/IStandaloneDataIterator.javaspock-core/src/main/java/org/spockframework/runtime/StandaloneDataIterationContext.javaspock-core/src/main/java/org/spockframework/runtime/StandaloneDataProviderDescriptor.javaspock-core/src/main/java/org/spockframework/runtime/StandaloneDataProviders.javaspock-core/src/main/java/org/spockframework/runtime/model/StandaloneDataProviderMetadata.javaspock-core/src/main/java/spock/lang/DataProvider.javaspock-specs/src/test/groovy/org/spockframework/docs/datadriven/DataSpec.groovyspock-specs/src/test/groovy/org/spockframework/smoke/parameterization/StandaloneDataProviderMethods.groovy
Comment on lines
+203
to
+205
| if (generics == null || generics.length != 1 || generics[0].isWildcard()) { | ||
| // raw Iterator (or an unbounded wildcard) is acceptable | ||
| return; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Groovy's org.codehaus.groovy.ast.GenericsType API, does isWildcard() return true for bounded wildcards like ? extends Foo and ? super Foo, with bounds exposed via getUpperBounds()/getLowerBound()?
💡 Result:
In Groovy's org.codehaus.groovy.ast.GenericsType API, the isWildcard method returns true for bounded wildcards such as? extends Foo and? super Foo [1][2]. The GenericsType class is specifically designed to represent these generic type definitions, and it provides the getUpperBounds and getLowerBound methods to expose the respective bounds of the wildcard [1][3][2]. You can inspect the bounds of a wildcard-enabled GenericsType instance using these methods once isWildcard returns true [1][2].
Citations:
- 1: https://docs.groovy-lang.org/docs/groovy-4.0.0/html/gapi/org/codehaus/groovy/ast/GenericsType.html
- 2: https://docs.groovy-lang.org/3.0.4/html/gapi/org/codehaus/groovy/ast/GenericsType.html
- 3: https://docs.groovy-lang.org/docs/groovy-3.0.7/html/api/org/codehaus/groovy/ast/GenericsType.html
Fix bounded-wildcard Iterator element types bypassing @DataProvider Tuple validation
handleReturnType() returns early for any generics[0].isWildcard(), and Groovy org.codehaus.groovy.ast.GenericsType.isWildcard() is true for bounded wildcards (? extends Foo / ? super Foo). As a result, Iterator<? extends ...> / Iterator<? super ...> can skip the subsequent tuple/arity contract checks; the early return should allow only an unbounded wildcard (e.g., Iterator<?>) and otherwise report an error (using getUpperBounds() / getLowerBound() to detect “unbounded” vs “bounded”).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@spock-core/src/main/java/org/spockframework/compiler/DataProviderMethodRewriter.java`
around lines 203 - 205, handleReturnType() currently returns for any
generics[0].isWildcard(), which incorrectly allows bounded wildcards; change the
early-return condition so it only returns for an unbounded wildcard
(Iterator<?>). Concretely, in DataProviderMethodRewriter.handleReturnType(),
replace the generics[0].isWildcard() check with a check that ensures the
wildcard has no explicit bounds (use generics[0].getUpperBounds() and
generics[0].getLowerBound() to detect an unbounded wildcard — e.g. upper bound
is Object or no meaningful upper bound and lower bound is null). If the wildcard
is bounded (has a non-Object upper bound or a non-null lower bound), do NOT
return early; instead report a validation error (use the class’s existing error
reporting helper used elsewhere in this rewriter, e.g., addError(...) or the
same mechanism used for other return-type errors) so Iterator<? extends
...>/Iterator<? super ...> do not bypass Tuple/arity checks.
Comment on lines
516
to
523
| if (dataVariableMultiplicationsMethod != null) { | ||
| try { | ||
| dataVariableMultiplications = Arrays.stream(((DataVariableMultiplication[]) invokeRaw(null, dataVariableMultiplicationsMethod))).iterator(); | ||
| nextDataVariableMultiplication = dataVariableMultiplications.next(); | ||
| } catch (Throwable t) { | ||
| supervisor.error(context.getErrorInfoCollector(), new ErrorInfo(dataVariableMultiplicationsMethod, t, getErrorContext())); | ||
| context.error(dataVariableMultiplicationsMethod, t); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Guard the multiplication-method result before streaming it.
If invokeRaw(...) fails in the feature path, it already records the real error and returns null. This block then calls Arrays.stream(...) on that null, which adds a second NullPointerException on top of the original failure and breaks the new IDataIterationContext record-and-return contract.
💡 Suggested fix
if (dataVariableMultiplicationsMethod != null) {
try {
- dataVariableMultiplications = Arrays.stream(((DataVariableMultiplication[]) invokeRaw(null, dataVariableMultiplicationsMethod))).iterator();
+ DataVariableMultiplication[] multiplications =
+ (DataVariableMultiplication[]) invokeRaw(null, dataVariableMultiplicationsMethod);
+ if (multiplications == null || context.hasErrors()) {
+ return null;
+ }
+ dataVariableMultiplications = Arrays.stream(multiplications).iterator();
nextDataVariableMultiplication = dataVariableMultiplications.next();
} catch (Throwable t) {
context.error(dataVariableMultiplicationsMethod, t);
return null;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spock-core/src/main/java/org/spockframework/runtime/DataIteratorFactory.java`
around lines 516 - 523, The code calls Arrays.stream(...) on the result of
invokeRaw(...) without checking for null; change the block in
DataIteratorFactory so you first capture the invokeRaw result into a temporary
DataVariableMultiplication[] (e.g. DataVariableMultiplication[] arr =
(DataVariableMultiplication[]) invokeRaw(null,
dataVariableMultiplicationsMethod)), then if arr == null return null immediately
(since invokeRaw already recorded the error), otherwise set
dataVariableMultiplications = Arrays.stream(arr).iterator() and
nextDataVariableMultiplication = dataVariableMultiplications.next(); keep the
existing try/catch around the invokeRaw/streaming to handle any other Throwable
and call context.error(...) there as before.
On the standalone path the iteration context rethrows errors instead of recording them, so a data provider failing during iterator chain construction aborted the DataProviderIterators constructor after the where-block variables had already been evaluated, leaking any AutoCloseable values. Release everything created so far (including partially created providers) before rethrowing; feature contexts record errors and never throw here, so that path is unaffected.
- Replace deprecated Class.newInstance() with a getDeclaredConstructor() based instantiate() helper and use verifyEach for per-row assertions. - Parse plain classes with the embedded compiler's per-test loader instead of constructing a dedicated GroovyClassLoader; compile() itself cannot be used because it filters its result to specifications. - Tidy imports.
leonard84
force-pushed
the
standalone-data-provider-methods
branch
from
edfa511 to
e892b42
Compare
Greptile SummaryThis PR adds standalone Confidence Score: 4/5Safe to merge; findings are non-blocking quality concerns with no correctness impact under current usage Well-structured implementation with thorough test coverage. Two P2 findings: a fragile UnsupportedOperationException in getStackTraceFilter() guarded only by a hard-coded false flag, and a documented non-volatile benign race in the tuple constructor cache. Neither blocks correctness in current code. StandaloneDataIterationContext.java (getStackTraceFilter UnsupportedOperationException), StandaloneDataProviders.java (non-volatile tuple constructor cache) Important Files Changed
Reviews (1): Last reviewed commit: "Polish standalone @DataProvider tests" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+120
to
+124
| } | ||
|
|
||
| @Override | ||
| public List<DataProviderInfo> getDataProviders() { | ||
| return dataProviders; |
There was a problem hiding this comment.
getStackTraceFilter() unconditionally throws UnsupportedOperationException, but the contract in IDataIterationContext says it is "only consulted when isLogFilteredIterations() is true". Since isLogFilteredIterations() always returns false here the throw will never be reached at runtime, but any future caller that adds a new call-site without checking the flag first will get an unexpected runtime explosion. Returning a no-op filter makes the class safe regardless of call order.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+146
to
+149
| // lazily resolved (benign race); the classes must not be referenced statically | ||
| // because not all of them exist on every supported Groovy version | ||
| private static final Constructor<?>[] TUPLE_CONSTRUCTORS = new Constructor<?>[HIGHEST_FIXED_TUPLE_ARITY + 1]; | ||
| private static final boolean[] TUPLE_CONSTRUCTOR_MISSING = new boolean[HIGHEST_FIXED_TUPLE_ARITY + 1]; |
There was a problem hiding this comment.
TUPLE_CONSTRUCTORS and TUPLE_CONSTRUCTOR_MISSING are plain (non-volatile) static arrays. The comment calls this a "benign race", but without a memory barrier a thread that writes TUPLE_CONSTRUCTORS[arity] may not have its write seen by other threads before they read TUPLE_CONSTRUCTOR_MISSING[arity], so two threads can both conclude the constructor is missing and both call resolveTupleConstructor. The duplicate work is harmless, but making both arrays volatile or using an AtomicReferenceArray / AtomicBooleanArray would eliminate the data race formally and avoid confusion in future maintenance.
A @dataProvider body is where-block content, so it may now open with an optional `where:` label to mirror a feature method's where-block. The body is split into blocks and grammar-checked by the same parser feature methods use: the block-detection logic is extracted from SpecParser into a shared BlockParser, and the @dataProvider rewriter injects a leading `where:` label (when the first statement has none) before delegating to it. Previously every statement label was silently ignored. Now only the where-block grammar is accepted (a leading `where:` opener plus the `and:`, `combined:` and `filter:` continuations); a feature-method label, a typo, or a misplaced `where:` is a compile error instead of being swallowed.
leonard84
force-pushed
the
standalone-data-provider-methods
branch
from
2eae3ee to
fbe9214
Compare
✅ All tests passed ✅🏷️ Commit: fbe9214 Learn more about TestLens at testlens.app. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Introduces
@DataProvidermethods: a method whose body is written exactly like the content of awhere:block, and whose call returns a fresh, lazyIteratorover the resulting rows. Each row is an arity-specific Groovy tuple (Tuple1,Tuple2, ...), so the provider can feed any feature through the existing multi-variable data pipe ([a, b] << provider()).This makes the full
where:block grammar reusable outside a single feature:combined:multiplications, and a trailingfilter:block;staticor instance, and may declare method parameters;where:block (@Shared/staticreadable, plain instance fields not);defproviders get an inferredIterator<TupleN>return type; an explicit return type must bejava.util.Iterator;AutoCloseableand closes anyAutoCloseablewhere-block local variables in reverse declaration order, including when consumed indirectly or via a feature's data pipe.Implementation notes
IDataIterationContext/IBaseDataIteratorfromDataIteratorFactoryso standalone providers and feature methods share the tuple-iteration machinery.WhereBlockRewriter, exposing the parsed providers/variables for reuse by the newDataProviderMethodRewriter/DataProviderMethodTransformation.Test plan
StandaloneDataProviderMethodssmoke spec (47 cases), all green.org.spockframework.smoke.parameterization.*andorg.spockframework.docs.datadriven.*suites pass (501 passed, 1 skipped, 0 failed), confirming existing where-block behavior is unaffected.🤖 Generated with Claude Code