Add IsolatedJVMTest category for parallel tests which needs fresh JVM (not reused) #26047
Conversation
Patras3
changed the title
Patras3
added
Source: Internal
|
PR builder passed in 59 minutes (without change in number of tests). |
| * Annotates tests which has to be run in isolated (forked and fresh) JVM by surefire plugin in parallel profile. | ||
| * By default ParallelJVMTest are reusing forked JVMs and this can lead to problems with a static context. | ||
| * Combination of ParallelJVMTest (for example from parent class) and IsolatedJVMTest categories will prevent issue | ||
| * with a static context. |
There was a problem hiding this comment.
What is the motivation for this category?
I understand it might be an easy way to achieve test isolation, but unfortunately, I think this is a dangerous tool to use. Forking the JVM for each such test is costly, for one test it will not matter, but such tests will inevitably accumulate over time and will worsen the situation with the slow PR builder and whole test suite.
What is the test we want to isolate in this case?
There was a problem hiding this comment.
I wanted to add this due to test from this discussion:
#26032 (comment)
but after comment from @k-jamroz :
@Patras3 your change will fix the problem with tests, but we support embedded HZ instances. Nothing prevents user to start many embedded instances in the same JVM with different config. Or is such usage unsupported?
I'm not sure now if this is a proper way.
I wanted to use this annotation only for cases when tests fails in pr builder due to this issue.
There was a problem hiding this comment.
Ok, will comment on that PR.
There was a problem hiding this comment.
also some tests are heavy and misbehaving: #24563, isolating them would at least prevent cascading test failures (or maybe also decrease the likelihood of problems with the test itself) until the root cause is fixed
There was a problem hiding this comment.
slow PR builder
such tests will be the first candidate to optimize or remove from PR builder suite
There was a problem hiding this comment.
It's true that this can be used in way, also for investigation if test failure is due test itself or due to others, but then it cannot be used to "close" issue and if it will be a temporary solution then probably this annotation will remain forever 😅
|
PR closed by Hazelcast automation as no activity (>3 months). Please reopen with comments, if necessary. Thank you for using Hazelcast and your valuable contributions |
Using this
IsolatedJVMTestcategory moves tests withParallelJVMTestto separate execution of surefire, still parallel but with not reused forked JVM.I tried this with my own version of #26032 (where we meet such issue with static context and pr builder) and it passed here:
Jenkins: https://jenkins.hazelcast.com/job/Hazelcast-pr-builder-sql-patryk/3/
Branch: https://github.com/Patras3/hazelcast/tree/fix/5.4/small-opt-perf
I think we should rewrite our categories and way of running tests in a future as some of tests without any category are invoked more than once, but this PR is just to fix issues with reused static context for some tests.
Checklist:
Team:,Type:,Source:,Module:) and Milestone setAdd to Release Noteslabel if changes should be mentioned in release notes orNot Release Notes contentif changes are not relevant for release notes