Support @StartStop fields in nested tests - #9614
Conversation
kdelay
left a comment
There was a problem hiding this comment.
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 (
@Nestedinside@Nested): outer, middle and innermost servers are all started. - Two
@StartStopfields referencing the sameMockWebServer: fine, sincestart()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
allInstancesdoes 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 withIllegalStateException: close() already called, becausebeforeEachrestarts a server that the previous test's store already closed. It reproduces identically onmain, so it is not introduced here. - The red
loomcheck isDuplexTest.duplexWithRedirectin:okhttp:allTests, a module this PR does not touch.main's ownloomruns 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.
|
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. |
Fixes #9546
Summary
@StartStopservers declared on enclosing instances for JUnit 5@NestedtestsValidation
StartStopTestregression./gradlew :mockwebserver3-junit5:checkgit diff --check