Skip to content

Support @StartStop fields in nested tests - #9614

Closed
arpitagarwal1301 wants to merge 1 commit into
lysine-dev:mainfrom
arpitagarwal1301:fix/9546-mockwebserver-startstop-field-on
Closed

Support @StartStop fields in nested tests#9614
arpitagarwal1301 wants to merge 1 commit into
lysine-dev:mainfrom
arpitagarwal1301:fix/9546-mockwebserver-startstop-field-on

Conversation

@arpitagarwal1301

Copy link
Copy Markdown
Contributor

Fixes #9546

Summary

  • start @StartStop servers declared on enclosing instances for JUnit 5 @Nested tests
  • discover fields from each matching test instance and retain each server independently in the extension store
  • cover startup, non-annotated fields, and after-test cleanup for a nested test

Validation

  • focused StartStopTest regression
  • ./gradlew :mockwebserver3-junit5:check
  • git diff --check

@kdelay kdelay left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I checked this out locally (bca7b58, JDK 21, macOS arm64) and it does what it says: :mockwebserver3-junit5:test is green on the branch, and restoring only the old StartStopExtension.beforeEach while keeping the new test fails 3 tests (NestedTest.happyPath, NestedTest.executionError, StartStopTest.executionError), so the new test really pins the behaviour.

One thing that may be worth calling out explicitly: the store-key change from field to server is load-bearing, not cosmetic, and nothing in the current test would catch it if it were reverted. It becomes observable as soon as the outer class and the @Nested class inherit the @StartStop field from a shared base:

abstract class SharedBase {
  @StartStop val server = MockWebServer().apply { dispatcher = ClosableDispatcher() }
}

class CollisionTest : SharedBase() {
  @Test fun outerTest() {}

  @Nested inner class Child : SharedBase() {
    @Test fun childTest() {}
  }
}

findAnnotatedFields returns the same java.lang.reflect.Field for both instances, so with store.put(field, server) the child's entry overwrites the outer's. I measured it: both servers start either way, but with the field key only the child's server is closed (outer dispatcher closed=false in an AfterAllCallback), while with store.put(server, server) both are closed. That is a leak the current diff fixes silently; a second test for it would keep it from regressing.

Other cases I ran against the branch, all behaving as expected:

  • Two levels of nesting (@Nested inside @Nested): outer, middle and innermost servers are all started.
  • Two @StartStop fields referencing the same MockWebServer: fine, since start() returns early for an already-bound address and the single store entry closes it once.
  • The outer instance's servers are closed after a nested test, so iterating allInstances does not leak in the plain case.

Two notes that are not about this change:

  • Unrelated pre-existing behaviour: @TestInstance(PER_CLASS) with two test methods fails with IllegalStateException: close() already called, because beforeEach restarts a server that the previous test's store already closed. It reproduces identically on main, so it is not introduced here.
  • The red loom check is DuplexTest.duplexWithRedirect in :okhttp:allTests, a module this PR does not touch. main's own loom runs are red too with a different set of tests each time (run 30692116591: BasicLoomTest/BasicMockServerTest/BasicProxyTest; run 30546012738: EventListenerTest_Call.successfulSecureConnect), which looks like existing flakiness on that leg rather than a regression from this branch.

@JakeWharton

Copy link
Copy Markdown
Collaborator

Hi! Thanks for the PR, but we are imminently about to ban LLM descriptions and comments, as well as LLM-authored code. Your PR description is pretty clearly LLM authored and that makes me think the code is too (I didn't review the diff).

You are welcome to use whatever tools you want for investing problems, but we expect all code to be written by hand along with comments and descriptions. If you can meet these requirements, you're welcome to make a new PR with a fix.

@JakeWharton JakeWharton closed this Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

MockWebServer @StartStop field on outer class not started for @Nested JUnit 5 tests

3 participants