Use TestInstance.Lifecycle.PER_CLASS in subclasses too#18750
Conversation
There was a problem hiding this comment.
Pull request overview
Converts the play-ws Play WS client test base classes from static state to instance state under @TestInstance(Lifecycle.PER_CLASS), and codifies the corresponding pattern in the module-cleanup agent and shared testing knowledge docs.
Changes:
- Add
@TestInstance(Lifecycle.PER_CLASS)toPlayWsClientBaseTestand its four concrete subclasses; convertstaticfixture fields and@BeforeAllmethods to instance form, and make theAutoCleanupExtensionan instance@RegisterExtensionfield. - Document the abstract-base + subclass
PER_CLASSlifecycle pattern (with instanceAutoCleanupExtension) intesting-general-patterns.mdand the module-cleanup agent instructions.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| instrumentation/play/play-ws/play-ws-common-1.0/testing/.../PlayWsClientBaseTest.java | Apply PER_CLASS, convert fixtures and AutoCleanupExtension to instance scope. |
| instrumentation/play/play-ws/play-ws-common-1.0/testing/.../PlayScalaWsClientBaseTest.java | Repeat PER_CLASS on subclass; convert fields and helpers to instance. |
| instrumentation/play/play-ws/play-ws-common-1.0/testing/.../PlayScalaStreamedWsClientBaseTest.java | Same conversion for the Scala streamed variant. |
| instrumentation/play/play-ws/play-ws-common-1.0/testing/.../PlayJavaWsClientBaseTest.java | Same conversion for the Java WS variant. |
| instrumentation/play/play-ws/play-ws-common-1.0/testing/.../PlayJavaStreamedWsClientBaseTest.java | Same conversion for the Java streamed variant. |
| .github/agents/module-cleanup.agent.md | Add guidance for abstract-base PER_CLASS + instance AutoCleanupExtension pattern. |
| .github/agents/knowledge/testing-general-patterns.md | Document the same pattern with rationale against shared static AutoCleanupExtension. |
| initialized earlier (e.g. a still-running test container). | ||
| - In abstract test bases that are inherited by multiple concrete test classes, | ||
| use `@TestInstance(Lifecycle.PER_CLASS)`, instance fixture fields, and an | ||
| instance `@RegisterExtension AutoCleanupExtension cleanup`. Register |
There was a problem hiding this comment.
intellij warns when @RegisterExtension (at least for InstrumentationExtension) is used on a non-static field. https://youtrack.jetbrains.com/projects/IDEA/issues/IDEA-362874/JUnit-5-false-positive-extension-should-be-registered-at-the-class-level explains that if you use a non-static field then beforeAll and afterAll callbacks won't work. Idk maybe this isn't an issue with @TestInstance(Lifecycle.PER_CLASS)
| import play.libs.ws.StandaloneWSResponse; | ||
| import play.libs.ws.ahc.StandaloneAhcWSClient; | ||
|
|
||
| @TestInstance(TestInstance.Lifecycle.PER_CLASS) |
There was a problem hiding this comment.
I think we'll need to add this to many classes. Not sure it is worth it shouldn't be hard to tell when it is missing and you need it.
Subclasses already inherit
TestInstance.Lifecycle.PER_CLASSfrom their parent, but (a) this is easy to miss since you have to look at the parent to know this, and (b) we are still usingstaticstate in many of them.This PR establishes the coding guidelines / pattern.
Will follow-up afterwards to apply to remaining locations.
Extracted from #18741