Support defining conditions in `@Verify`/`@VerifyAll` helper methods #2112

pshevche · 2025-02-24T19:33:14Z

Description

The PR adds two new annotations: @spock.lang.Verify and @spock.lang.VerifyAll that can be applied to helper methods containing condition blocks. Top-level implicit or explicit assertion statements in these methods will be rewritten using the ConditionRewriter. @Verify and @VerifyAll helpers can be defined in any class, not only in the ones extending from Specification. The method is required to have a void return type.

Reuse the `ConditionRewriter` to transform condition expressions in methods annotated with `spock.lang.Verify`. The method can be defined in any class, not only the ones extending from `Specification`. The method is required to have a `void` return type. Implicit condition assertions on all levels will be transformed Signed-off-by: Pavlo Shevchenko <[email protected]>

Reuse the `VerifyMethodsRewriter` to rewrite implicit conditions in helper methods annotated with `@VerifyAll`. The only difference is to use `ErrorCollector` instead of `ErrorRethrower` to not rethrow the exception immediately, but to collect all failures and throw them at the end. Signed-off-by: Pavlo Shevchenko <[email protected]>

Signed-off-by: Pavlo Shevchenko <[email protected]>

spock-core/src/main/java/org/spockframework/compiler/condition/BaseVerifyMethodRewriter.java

Signed-off-by: Pavlo Shevchenko <[email protected]>

…FailedWithExceptionError` Signed-off-by: Pavlo Shevchenko <[email protected]>

Signed-off-by: Pavlo Shevchenko <[email protected]>

spock-core/src/main/java/org/spockframework/runtime/SpockRuntime.java

pshevche · 2025-02-24T19:48:14Z

spock-core/src/main/java/spock/lang/Snapshotter.java

@@ -276,7 +277,7 @@ public Store(IterationInfo iterationInfo, Path rootPath, boolean updateSnapshots
      this.extension = Assert.notNull(extension);
      this.charset = Assert.notNull(charset);

-      Class<?> specClass = iterationInfo.getFeature().getSpec().getReflection();
+      Class<?> specClass = iterationInfo.getFeature().getParent().getBottomSpec().getReflection();


💬 Allows defining snapshot-based in base classes, but child specs can overwrite snapshots.

This is a potentially breaking change, but it hasn't been in a final release yet. So it should be fine. We should document it in the release notes nonetheless.

I added release notes in 49d2685 (#2112). I think there is value in supporting both scenarios: you may want to define features and snapshots for base class, but overwrite them for certain features.

spock-core/src/main/java/spock/lang/Verify.java

codecov · 2025-02-24T19:56:05Z

Codecov Report

Attention: Patch coverage is 93.49593% with 8 lines in your changes missing coverage. Please review.

Project coverage is 82.03%. Comparing base (bc82305) to head (49d2685).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
.../compiler/condition/BaseVerifyMethodTransform.java	85.00%	3 Missing ⚠️
.../java/org/spockframework/runtime/SpockRuntime.java	50.00%	2 Missing ⚠️
...k/compiler/condition/BaseVerifyMethodRewriter.java	96.66%	0 Missing and 1 partial ⚠️
...rk/compiler/condition/ImplicitConditionsUtils.java	85.71%	0 Missing and 1 partial ⚠️
...ock-core/src/main/java/spock/lang/Snapshotter.java	50.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2112      +/-   ##
============================================
+ Coverage     81.90%   82.03%   +0.12%     
- Complexity     4692     4728      +36     
============================================
  Files           452      463      +11     
  Lines         14696    14771      +75     
  Branches       1852     1856       +4     
============================================
+ Hits          12037    12117      +80     
+ Misses         1975     1974       -1     
+ Partials        684      680       -4

Files with missing lines	Coverage Δ
...org/spockframework/compiler/ConditionRewriter.java	`90.54% <100.00%> (+0.04%)`	⬆️
...org/spockframework/compiler/DeepBlockRewriter.java	`98.77% <100.00%> (+0.53%)`	⬆️
...g/spockframework/compiler/InteractionRewriter.java	`91.79% <100.00%> (ø)`
...java/org/spockframework/compiler/SpecRewriter.java	`93.76% <100.00%> (+0.06%)`	⬆️
...rg/spockframework/compiler/WhereBlockRewriter.java	`94.74% <100.00%> (ø)`
...iler/condition/DefaultConditionErrorRecorders.java	`100.00% <100.00%> (ø)`
...r/condition/DefaultConditionRewriterResources.java	`100.00% <100.00%> (ø)`
...k/compiler/condition/IConditionErrorRecorders.java	`100.00% <100.00%> (ø)`
...rk/compiler/condition/VerifyAllMethodRewriter.java	`100.00% <100.00%> (ø)`
...k/compiler/condition/VerifyAllMethodTransform.java	`100.00% <100.00%> (ø)`
... and 9 more

... and 7 files with indirect coverage changes

...smoke/ast/condition/VerifyAllMethodsAstSpec/ignores_conditions_in_overwritten_methods.groovy

spock-core/src/main/java/org/spockframework/compiler/HelperMethodsVisitor.java

spock-core/src/main/java/org/spockframework/runtime/SpockRuntime.java

leonard84 · 2025-02-25T15:51:12Z

spock-core/src/main/java/spock/lang/Snapshotter.java

@@ -276,7 +277,7 @@ public Store(IterationInfo iterationInfo, Path rootPath, boolean updateSnapshots
      this.extension = Assert.notNull(extension);
      this.charset = Assert.notNull(charset);

-      Class<?> specClass = iterationInfo.getFeature().getSpec().getReflection();
+      Class<?> specClass = iterationInfo.getFeature().getParent().getBottomSpec().getReflection();


This is a potentially breaking change, but it hasn't been in a final release yet. So it should be fine. We should document it in the release notes nonetheless.

spock-core/src/main/java/spock/lang/Verify.java

spock-core/src/main/java/spock/lang/VerifyAll.java

spock-specs/src/test/groovy/org/spockframework/smoke/VerifyAllMethodsSpecification.groovy

spock-specs/src/test/groovy/org/spockframework/smoke/VerifyMethodsSpecification.groovy

...smoke/ast/condition/VerifyAllMethodsAstSpec/ignores_conditions_in_overwritten_methods.groovy

...specs/src/test/groovy/org/spockframework/smoke/ast/condition/BaseVerifyMethodsAstSpec.groovy

…s-in-helper-methods

…s thrown by our ErrorCollector

leonard84 · 2025-02-28T15:17:38Z

...specs/src/test/groovy/org/spockframework/smoke/ast/condition/BaseVerifyMethodsAstSpec.groovy

+    snapshotter.assertThat(result.source).matchesSnapshot()
+  }
+
+  def "transforms only top-level conditions"() {


I wonder if we want to keep that behavior for these annotations. It is a classic foot gun. If it wouldn't break anything I would probably do away with that restriction in the Spec itself. However, that would have to wait until Spock 3.

Inside with or verifyAll we already support assertions in if and so on.

Thoughts: @Vampire, @AndreasTu ?

I would not keep this behavior for the new feature, if it is simple to change it for the new one and preserve the old behavior in specs until Spock 3.

leonard84 · 2025-02-28T15:19:28Z

spock-core/src/main/java/org/spockframework/compiler/condition/BaseVerifyMethodTransform.java

+        processVerificationHelperMethod((MethodNode) node, errorReporter, sourceLookup);
+      }
+    } finally {
+      sourceLookup.close();


🤔 should we make SourceLookup AutoClosable so we could use it in try-with?

leonard84 · 2025-02-28T15:21:10Z

spock-specs/src/test/groovy/org/spockframework/smoke/VerifyAllMethodsSpecification.groovy

+
+    then:
+    result.failures.size() == 1
+    with(result.failures[0].exception, MultipleFailuresError) {


Isn't it

Suggested change

with(result.failures[0].exception, MultipleFailuresError) {

with(result.failures[0].exception, SpockMultipleFailuresError) {

AndreasTu

Thanks a lot for your work! 👍
I like the idea for these annotations.

AndreasTu · 2025-02-28T17:00:09Z

spock-core/src/main/java/spock/lang/Verify.java

+ * may contain conditions, allowing to omit the {@code assert} keyword.
+ * As in expect-blocks and then-blocks, variable declarations
+ * and void method invocations will not be considered conditions.
+ * The method can be defined in the {@link Specification} class or in a separate class.


Maybe add that it needs to be a Groovy class.

Suggested change

* The method can be defined in the {@link Specification} class or in a separate class.

* The method can be defined in the {@link Specification} class or in a separate Groovy class.

AndreasTu · 2025-02-28T17:00:51Z

spock-core/src/main/java/spock/lang/Verify.java

+ * and void method invocations will not be considered conditions.
+ * The method can be defined in the {@link Specification} class or in a separate class.
+ * The method must have a {@code void} return type.
+ * The test will fail on the first failing assertion.


Suggested change

* The test will fail on the first failing assertion.

* A test calling the method will fail on the first failing assertion.

AndreasTu · 2025-02-28T17:01:12Z

spock-core/src/main/java/spock/lang/VerifyAll.java

+ * may contain conditions, allowing to omit the {@code assert} keyword.
+ * As in expect-blocks and then-blocks, variable declarations
+ * and void method invocations will not be considered conditions.
+ * The method can be defined inside the {@link Specification} class or in a separate class.


Suggested change

* The method can be defined inside the {@link Specification} class or in a separate class.

* The method can be defined inside the {@link Specification} class or in a separate Groovy class.

AndreasTu · 2025-02-28T17:05:25Z

...specs/src/test/groovy/org/spockframework/smoke/ast/condition/BaseVerifyMethodsAstSpec.groovy

+    snapshotter.assertThat(result.source).matchesSnapshot()
+  }
+
+  def "transforms only top-level conditions"() {


I would not keep this behavior for the new feature, if it is simple to change it for the new one and preserve the old behavior in specs until Spock 3.

AndreasTu · 2025-02-28T17:10:22Z

spock-core/src/main/java/org/spockframework/runtime/ErrorCollector.java

@@ -23,7 +21,7 @@ public void validateCollectedErrors() throws Throwable {
        throw throwables.get(0);

      default:
-        throw new MultipleFailuresError("", throwables);
+        throw new SpockMultipleFailuresError("", throwables);


Maybe document in the release_notes that now the subclass SpockMultipleFailuresError is thrown instead of MultipleFailuresError.
Sometimes people do strange checks, and this would at least docoment the change, although it is a compatible one.

AndreasTu · 2025-02-28T17:12:42Z

...e/src/main/java/org/spockframework/compiler/condition/DefaultConditionRewriterResources.java

+    this.nodeCache = nodeCache;
+    this.lookup = lookup;
+    this.errorReporter = errorReporter;
+    this.errorRecorders = errorRecorders;


Please add Objects.requireNonNull() to indicate non-null params or add @Nulable annotations.

Suggested change

this.nodeCache = nodeCache;

this.lookup = lookup;

this.errorReporter = errorReporter;

this.errorRecorders = errorRecorders;

this.nodeCache = requireNonNull(nodeCache);

this.lookup = requireNonNull(lookup);

this.errorReporter = requireNonNull(errorReporter);

this.errorRecorders = requireNonNull(errorRecorders);

For internal code it should be org.spockframework.util.Assert#notNull(T) and for user facing code it would be org.spockframework.util.Checks#notNull.

AndreasTu · 2025-02-28T17:13:03Z

...core/src/main/java/org/spockframework/compiler/condition/DefaultConditionErrorRecorders.java

+  private final AstNodeCache nodeCache;
+
+  public DefaultConditionErrorRecorders(AstNodeCache nodeCache) {
+    this.nodeCache = nodeCache;


Suggested change

this.nodeCache = nodeCache;

this.nodeCache = requireNonNull(nodeCache);

AndreasTu · 2025-02-28T17:15:05Z

spock-core/src/main/java/org/spockframework/compiler/condition/BaseVerifyMethodRewriter.java

+    this.resources = resources;
+    this.methodNode = methodNode;


Suggested change

this.resources = resources;

this.methodNode = methodNode;

this.resources = requireNonNull(resources);

this.methodNode = requireNonNull(methodNode);

pshevche added 7 commits February 22, 2025 23:31

Support using snapshotter in base specification classes

efa9d5a

Signed-off-by: Pavlo Shevchenko <[email protected]>

Fix formatting of BaseVerifyMethodRewriter

1b0382a

Signed-off-by: Pavlo Shevchenko <[email protected]>

Document @Verify annotation

932eb06

Signed-off-by: Pavlo Shevchenko <[email protected]>

Document @VerifyAll annotation

3be89cd

Signed-off-by: Pavlo Shevchenko <[email protected]>

Fix mis-formatted import statements

2604e74

Signed-off-by: Pavlo Shevchenko <[email protected]>

pshevche force-pushed the pshevche/implicit-assertions-in-helper-methods branch from cf60234 to 1ccb09b Compare February 24, 2025 19:38

pshevche commented Feb 24, 2025

View reviewed changes

spock-core/src/main/java/org/spockframework/compiler/condition/BaseVerifyMethodRewriter.java Show resolved Hide resolved

pshevche added 3 commits February 24, 2025 20:41

Transform only top-level implicit conditions in helper methods

3aed74d

Signed-off-by: Pavlo Shevchenko <[email protected]>

Prevent assertions in helper methods from being wrapped in `Condition…

9516593

…FailedWithExceptionError` Signed-off-by: Pavlo Shevchenko <[email protected]>

Revert unintentional formatting changes

ebe0044

Signed-off-by: Pavlo Shevchenko <[email protected]>

pshevche force-pushed the pshevche/implicit-assertions-in-helper-methods branch from 1ccb09b to ebe0044 Compare February 24, 2025 19:41

Add missing copyright statements

17d633c

Signed-off-by: Pavlo Shevchenko <[email protected]>

pshevche commented Feb 24, 2025

View reviewed changes

spock-core/src/main/java/org/spockframework/runtime/SpockRuntime.java Outdated Show resolved Hide resolved

pshevche commented Feb 24, 2025

View reviewed changes

spock-core/src/main/java/spock/lang/Verify.java Outdated Show resolved Hide resolved

pshevche commented Feb 24, 2025

View reviewed changes

...smoke/ast/condition/VerifyAllMethodsAstSpec/ignores_conditions_in_overwritten_methods.groovy Show resolved Hide resolved

leonard84 requested changes Feb 25, 2025

View reviewed changes

leonard84 added this to the 2.4-M6 milestone Feb 25, 2025

leonard84 self-assigned this Feb 25, 2025

pshevche added 8 commits February 28, 2025 09:10

Merge branch 'spockframework:master' into pshevche/implicit-assertion…

aecc6e4

…s-in-helper-methods

Define a SpockMultipleFailuresError to identify assertion exception…

34062b5

…s thrown by our ErrorCollector

Mark Verify(All) annotations as beta and fix up JavaDoc

145244e

Use parameter types in documentation tests for @Verify(All)

70e0fe8

Do not use EmbeddedSpecification unless necessary

5913512

Use a dedicated transform for rewriting verification helper methods

31e6d74

Fail if the @Verify(All) method has a non-void return type

6875970

Add release notes

49d2685

pshevche requested a review from leonard84 February 28, 2025 09:50

leonard84 reviewed Feb 28, 2025

View reviewed changes

AndreasTu reviewed Feb 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support defining conditions in `@Verify`/`@VerifyAll` helper methods #2112

Support defining conditions in `@Verify`/`@VerifyAll` helper methods #2112

pshevche commented Feb 24, 2025

pshevche Feb 24, 2025

leonard84 Feb 25, 2025

pshevche Feb 28, 2025

codecov bot commented Feb 24, 2025 •

edited

Loading

leonard84 Feb 25, 2025

leonard84 Feb 28, 2025

AndreasTu Feb 28, 2025

leonard84 Feb 28, 2025

leonard84 Feb 28, 2025

AndreasTu left a comment

AndreasTu Feb 28, 2025

AndreasTu Feb 28, 2025

AndreasTu Feb 28, 2025

AndreasTu Feb 28, 2025

AndreasTu Feb 28, 2025

AndreasTu Feb 28, 2025

leonard84 Mar 1, 2025

AndreasTu Feb 28, 2025

AndreasTu Feb 28, 2025

	with(result.failures[0].exception, MultipleFailuresError) {
	with(result.failures[0].exception, SpockMultipleFailuresError) {

	* The method can be defined in the {@link Specification} class or in a separate class.
	* The method can be defined in the {@link Specification} class or in a separate Groovy class.

	* The test will fail on the first failing assertion.
	* A test calling the method will fail on the first failing assertion.

	* The method can be defined inside the {@link Specification} class or in a separate class.
	* The method can be defined inside the {@link Specification} class or in a separate Groovy class.

	this.nodeCache = nodeCache;
	this.nodeCache = requireNonNull(nodeCache);

Support defining conditions in @Verify/@VerifyAll helper methods #2112

Are you sure you want to change the base?

Support defining conditions in @Verify/@VerifyAll helper methods #2112

Conversation

pshevche commented Feb 24, 2025

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 24, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasTu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Support defining conditions in `@Verify`/`@VerifyAll` helper methods #2112

Support defining conditions in `@Verify`/`@VerifyAll` helper methods #2112

codecov bot commented Feb 24, 2025 •

edited

Loading