Skip to content

Extend the top interaction scope to cleanup methods#2366

Open
Vampire wants to merge 3 commits into
masterfrom
vampire/extend-top-scope-to-cleanup-method
Open

Extend the top interaction scope to cleanup methods#2366
Vampire wants to merge 3 commits into
masterfrom
vampire/extend-top-scope-to-cleanup-method

Conversation

@Vampire

@Vampire Vampire commented Jun 2, 2026

Copy link
Copy Markdown
Member

Fixes #616

@coderabbitai

coderabbitai Bot commented Jun 2, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Interactions declared outside then: now stay active through cleanup, and feature-method exits now invoke verifyLastScope() instead of leaveScope(). Documentation, release notes, a smoke spec, and generated snapshots were updated to match.

Changes

Global interactions now verified at cleanup

Layer / File(s) Summary
MockController API extension
spock-core/src/main/java/org/spockframework/mock/runtime/MockController.java
Adds VERIFY_LAST_SCOPE and verifyLastScope(), and switches to Assert.
Compiler wiring and defensive copy
spock-core/src/main/java/org/spockframework/compiler/AstNodeCache.java, spock-core/src/main/java/org/spockframework/compiler/SpecRewriter.java, spock-core/src/main/java/org/spockframework/mock/TooFewInvocationsError.java
Caches the new method node, emits verifyLastScope() at feature exits, and copies unmatchedInvocations defensively.
Docs and release notes
docs/interaction_based_testing.adoc, docs/release_notes.adoc
Updates the interaction scope description and adds the 2.5 breaking-change note.
Smoke test for cleanup verification
spock-specs/src/test/groovy/org/spockframework/smoke/mock/GlobalInteractionsInCleanup.groovy
Adds the cleanup-focused spec with passing and failing cases.
Generated snapshot updates
spock-specs/src/test/resources/snapshots/**
Replaces leaveScope() with verifyLastScope() across generated AST and spec snapshots.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: AndreasTu

Poem

🐰 I hop by cleanup, soft and bright,
The last scope shines with verifying light.
Stubs stay awake till the end of play,
Then leap to verifyLastScope() and dart away.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The defensive copy change in TooFewInvocationsError appears unrelated to the cleanup-scope fix in issue #616. Remove that change from this PR or split it into a separate, clearly linked bug fix.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: extending interaction scope through cleanup methods.
Description check ✅ Passed The description is related to the change set and references the fixed issue.
Linked Issues check ✅ Passed [#616] The PR keeps stubbed interactions available in cleanup and adds coverage for the reported success case.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Vampire commented Jun 2, 2026

Copy link
Copy Markdown
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@greptile-apps

greptile-apps Bot commented Jun 2, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes issue #616 by keeping the global (base) interaction scope alive through the cleanup() fixture method instead of removing it at the end of the feature method. The mechanism replaces the leaveScope() call injected at the end of generated feature method code with a new verifyLastScope() call that verifies lower-cardinality constraints without popping the scope off the deque, so stubs remain accessible when the Spock runner invokes cleanup() after the feature method returns.

  • MockController.verifyLastScope() is added as a peer to leaveScope(); it asserts the deque has exactly one remaining scope, verifies interactions, but does not remove the scope — enabling the cleanup() fixture method to resolve stub responses from previously declared global interactions.
  • TooFewInvocationsError now takes a defensive copy of unmatchedInvocations to guard against post-verification mutations (e.g. cleanup method invocations adding to the list after the error is constructed while the synchronized lock is held).
  • AstNodeCache is extended to cache the new verifyLastScope MethodNode, and all affected AST snapshots are updated accordingly.

Confidence Score: 4/5

The change is safe to merge; the core logic is correct and the scope-lifetime fix is well-reasoned.

The mechanism — keeping the base InteractionScope alive via verifyLastScope() instead of removing it with leaveScope() — is correctly implemented. The defensive copy in TooFewInvocationsError protects against post-verification list mutations, and the Assert guard in verifyLastScope() is unreachable in all normal execution paths. The only gaps are a minor style nit, a documentation ambiguity around cleanup: blocks, and missing test coverage for the upper-cardinality breaking change called out in the release notes.

MockController.java and GlobalInteractionsInCleanup.groovy deserve a second look — the former for the minor style nit, the latter for the missing upper-cardinality test case.

Important Files Changed

Filename Overview
spock-core/src/main/java/org/spockframework/mock/runtime/MockController.java Adds verifyLastScope() that verifies the base scope without removing it; a double-space nit is present in the Assert.that call.
spock-core/src/main/java/org/spockframework/compiler/SpecRewriter.java Replaces MockController_LeaveScope with MockController_VerifyLastScope for FeatureMethod in visitMethodAgain; position relative to cleanup is correct.
spock-core/src/main/java/org/spockframework/mock/TooFewInvocationsError.java Adds a defensive copy of unmatchedInvocations to prevent post-verification mutations from corrupting the error's diagnostic data.
spock-specs/src/test/groovy/org/spockframework/smoke/mock/GlobalInteractionsInCleanup.groovy New spec exercising the feature; covers the happy path and test-failure path, but lacks coverage for the upper-cardinality-exceeded-in-cleanup breaking change.
docs/interaction_based_testing.adoc Single-sentence update extending scope lifetime to the cleanup method; could be more precise about cleanup: blocks vs the cleanup() fixture method.

Sequence Diagram

sequenceDiagram
    participant Runner as Spock Runner
    participant Feature as Feature Method
    participant MC as MockController
    participant Scope as InteractionScope (base)
    participant Cleanup as cleanup() Fixture Method

    Runner->>MC: "new MockController() — addFirst(InteractionScope)"
    Runner->>Feature: "setup() adds interactions via addInteraction()"
    activate Scope
    Runner->>Feature: "execute feature body (given/when/then/expect)"
    Note over Feature,Scope: Stubs active during feature body
    Feature->>MC: "verifyLastScope() [NEW — replaces leaveScope()]"
    MC->>Scope: "verifyInteractions() — lower cardinality check"
    Note over MC,Scope: Scope NOT removed — stays in deque
    Feature-->>Runner: "return (scope still live)"
    Runner->>Cleanup: "cleanup() fixture method executes"
    Cleanup->>MC: "handle(invocation)"
    MC->>Scope: "match(invocation) — stub response returned"
    Note over Scope: Upper cardinality still enforced (breaking change)
    Cleanup-->>Runner: done
    deactivate Scope
Loading

Reviews (1): Last reviewed commit: "Extend the top interaction scope to clea..." | Re-trigger Greptile

Comment thread spock-core/src/main/java/org/spockframework/mock/runtime/MockController.java Outdated
Comment thread docs/interaction_based_testing.adoc Outdated
@Vampire Vampire force-pushed the vampire/extend-top-scope-to-cleanup-method branch 2 times, most recently from 46eccf5 to 14b9ff9 Compare June 3, 2026 13:08
@codecov

codecov Bot commented Jun 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 82.33%. Comparing base (87e325a) to head (ce46df1).

Files with missing lines Patch % Lines
...rg/spockframework/mock/runtime/MockController.java 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2366   +/-   ##
=========================================
  Coverage     82.32%   82.33%           
- Complexity     4877     4878    +1     
=========================================
  Files           474      474           
  Lines         15205    15211    +6     
  Branches       1935     1935           
=========================================
+ Hits          12518    12524    +6     
  Misses         1993     1993           
  Partials        694      694           
Files with missing lines Coverage Δ
...java/org/spockframework/compiler/AstNodeCache.java 100.00% <100.00%> (ø)
...java/org/spockframework/compiler/SpecRewriter.java 94.17% <100.00%> (ø)
...rg/spockframework/mock/TooFewInvocationsError.java 96.00% <100.00%> (ø)
...rg/spockframework/mock/runtime/MockController.java 96.38% <75.00%> (-1.09%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@leonard84 leonard84 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I would rephrase it slightly, as there might be more than one cleanup() method.

Comment thread docs/interaction_based_testing.adoc Outdated
Comment thread docs/release_notes.adoc Outdated

Vampire commented Jun 11, 2026

Copy link
Copy Markdown
Member Author

Totally right, thanks.
I also extended the test to have super-spec with cleanup method.

@Vampire Vampire requested a review from leonard84 June 11, 2026 13:26
@Vampire Vampire force-pushed the vampire/extend-top-scope-to-cleanup-method branch from 0477854 to 040a6c0 Compare July 8, 2026 09:44
@testlens-app

testlens-app Bot commented Jul 8, 2026

Copy link
Copy Markdown

✅ All tests passed ✅

🏷️ Commit: ce46df1
▶️ Tests: 69983 executed
⚪️ Checks: 61/61 completed


Learn more about TestLens at testlens.app.

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.

Cannot use stubbed methods in cleanup method if the test succeeds

2 participants