Defer reactive cleanup on testServer() exit for the 1.14.0 release#4396
Open
cpsievert wants to merge 5 commits into
Open
Defer reactive cleanup on testServer() exit for the 1.14.0 release#4396cpsievert wants to merge 5 commits into
cpsievert wants to merge 5 commits into
Conversation
#4372 made a closing session destroy its bound reactives. testServer() closes its mock session on exit, so reactives created inside a module under test are destroyed as soon as the block returns. Several CRAN packages (blockr.core, blockr.dock, teal.slice) read such reactives after the testServer() block and broke with destroyedReactiveError. Comment out invokeDestroyCallbacks("") in MockShinySession$close() for the 1.14.0 release so the test harness reverts to pre-#4372 behavior. The real ShinySession still destroys on close, so app behavior is unchanged. A detailed note in the source explains how/when to re-enable after notifying the affected maintainers; the paired close-destroys tests are commented out with the same marker.
cpsievert
commented
Jun 1, 2026
cpsievert
commented
Jun 1, 2026
cpsievert
commented
Jun 1, 2026
This was referenced Jun 1, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Temporarily restores pre-#4372 testServer() behavior by preventing MockShinySession$close() from destroying session-bound reactives, avoiding breakage in downstream tests that read module reactives after the testServer() block.
Changes:
- Commented out
private$invokeDestroyCallbacks(allowRoot = TRUE)inMockShinySession$close()with a re-enable note for a future release. - Commented out tests asserting that
MockShinySession$close()invokes destroy callbacks.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
R/mock-session.R |
Disables destroy-callback invocation on mock session close (test harness only) and documents re-enable plan. |
tests/testthat/test-destroy.R |
Disables close/destroy-related expectations that no longer hold with the temporary mock-session behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+543
to
+547
| # RE-ENABLE alongside MockShinySession$close()'s invokeDestroyCallbacks("") call | ||
| # (see the note in R/mock-session.R). These tests assert that closing the mock | ||
| # session destroys its reactives; that behavior is temporarily disabled for the | ||
| # 1.14.0 release, so they are commented out and should be restored together with | ||
| # that line. |
| session$close() | ||
| expect_equal(order, c("child", "parent", "root")) | ||
| }) | ||
| # RE-ENABLE alongside MockShinySession$close()'s invokeDestroyCallbacks("") call |
Comment on lines
+585
to
+586
| # RE-ENABLE alongside MockShinySession$close()'s invokeDestroyCallbacks("") call | ||
| # (see the note in R/mock-session.R); disabled for the 1.14.0 release. |
Comment on lines
+371
to
+372
| # paired tests in tests/testthat/test-destroy.R (search for this same | ||
| # marker) and tests/testthat/test-test-server.R. |
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
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.
Why this matters
#4372 makes a closing session destroy the reactives bound to it. Because
testServer()closes its mock session on exit, reactives created inside a module under test are now torn down the moment thetestServer()block returns. Three CRAN reverse dependencies — blockr.core, blockr.dock, and teal.slice — read such reactives after thetestServer()block and now fail with"... its module session has been destroyed".To avoid breaking them in the 1.14.0 release, this temporarily reverts the test harness to its pre-
#4372behavior of leaving reactives intact on close.Scope and safety
The change comments out a single line —
invokeDestroyCallbacks("")— inMockShinySession$close(). The realShinySession(ShinySession$wsClosed()) still destroys reactives on close, so application behavior is unchanged; onlytestServer()/MockShinySessionis affected.This is deliberately temporary. A detailed note at the change site explains how and when to re-enable: once the affected maintainers have been notified to adapt their tests (create session-independent reactives with
withReactiveDomain(NULL, ...), or read/snapshot reactive state inside thetestServer()block), uncomment the line and the paired tests, which are commented out behind the same searchable marker intests/testthat/test-destroy.Randtests/testthat/test-test-server.R.Verification
A new test in
test-test-server.Rreproduces the failure (areactiveValuescreated inside a module is read aftertestServer()returns) and passes with the change. Both downstream failure shapes (returning a reactive from atestServer()helper; an external object holding a reactive created insidetestServer()) were confirmed fixed against this branch.