[Transform] Stop transforms at the end of tests#139783
[Transform] Stop transforms at the end of tests#139783prwhelan merged 8 commits intoelastic:mainfrom
Conversation
Stopping the transform at the end of test, before the reset, can help other tests running in parallel. Resolve elastic#122980
prwhelan
added
>test
elasticsearchmachine
added
v9.4.0
v9.1.10
v9.2.4
v8.19.10
v9.3.1
and removed
branch:9.2
branch:9.1
branch:8.19
branch:9.3
labels
There was a problem hiding this comment.
Pull request overview
This PR adds cleanup logic to stop transforms at the end of a test method to prevent interference with parallel test execution. The change addresses issue #122980 by ensuring transforms are properly stopped before test cleanup and reset operations.
Key Changes
- Added explicit
stopTransformcalls for both the main transform and its clone at the end of theupdateTransferRightsTestertest method
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
elasticsearchmachine
added
v8.19.11
v9.2.5
v9.1.11
and removed
v8.19.10
v9.2.4
v9.1.10
labels
|
Pinging @elastic/ml-core (Team:ML) |
| }, 15, TimeUnit.SECONDS); | ||
|
|
||
| stopTransform(transformId, BASIC_AUTH_VALUE_TRANSFORM_ADMIN_1, true, false); | ||
| stopTransform(transformIdCloned, BASIC_AUTH_VALUE_TRANSFORM_ADMIN_1, true, false); |
There was a problem hiding this comment.
No need to deleteTransform?
There was a problem hiding this comment.
I don't think so? But let's add it just to be safe, it'll keep a consistent pattern across the testing frameworks
benwtrent
left a comment
benwtrent
left a comment
There was a problem hiding this comment.
I am not blocking this change, but generally, if you need to do this, you should add an @After teardown type of thing to the tests to clear up any created transforms. Otherwise, if something throws inside the test, the transform will not be cleaned up (idk if we care about this or not).
A common pattern is to have some deque that keeps track of the IDs successfully created and then running through that deque in @After to clear up any created transforms.
While I normally agree, given how deprioritized Transforms is generally, I tend to lean towards smaller practical changes that are more likely to be reviewed and merged than broader changes that never get PR'ed. Even this PR took over 3 months. And if the test fails, there may be other tests complaining about the leaked resource, but the top-most failure is still this test's failure. |
Stopping the transform at the end of test, before the reset, can help other tests running in parallel. Resolve elastic#122980
Stopping the transform at the end of test, before the reset, can help other tests running in parallel. Resolve elastic#122980
Stopping the transform at the end of test, before the reset, can help other tests running in parallel. Resolve elastic#122980
Stopping the transform at the end of test, before the reset, can help other tests running in parallel. Resolve elastic#122980
Stopping the transform at the end of test, before the reset, can help other tests running in parallel. Resolve elastic#122980
…locations * upstream/main: (51 commits) ESQL: Remaining serialization tests (elastic#143470) Eagerly release resources in `TransportAwaitClusterStateVersionAppliedAction` (elastic#143477) Stop and relocate sliced reindex on shutdown (elastic#143183) Documentation for query_vector base64 parameter (elastic#142675) ES|QL: Fix LIMIT after all columns are dropped (elastic#143463) Update docs-build.yml (elastic#142958) Fix KnnIndexTester to work with byte vectors (elastic#143493) Fix IndexInputUtils.withSlice to produce native-safe MemorySegments on Java 21 (elastic#143479) CPS fix: include only relevant projects in the search response metadata (elastic#143367) apm-data: explicit map of timestamp.us to long (elastic#143173) [Inference API] Add custom headers for Azure OpenAI Service (elastic#142969) ESQL: Add name IDs to golden tests and fix synthetic names (elastic#143450) Add getUnavailableShards to BaseBroadcastResponse (elastic#143406) Add description to reindex API without sensitive info (elastic#143112) SQL: fix CLI tests (elastic#143451) ES|QL: Add note of future removal of FORK implicit LIMIT (elastic#143457) [Test] Randomly disable doc values skippers in time-series indices (elastic#143389) Improve pattern text downgrade license test (elastic#143102) [Transform] Stop transforms at the end of tests (elastic#139783) Mute org.elasticsearch.compute.lucene.read.ValueSourceReaderTypeConversionTests testLoadAll elastic#143471 ...
Stopping the transform at the end of test, before the reset, can help other tests running in parallel. Resolve elastic#122980
Stopping the transform at the end of test, before the reset, can help other tests running in parallel.
Resolve #122980