Conversation
elasticsearchmachine
added
needs:triage
mosche
left a comment
mosche
left a comment
There was a problem hiding this comment.
lgtm, just a small nit re adding more utilities to ESTestCase
| * @param key The key of the system property | ||
| */ | ||
| @SuppressForbidden(reason = "Uses System.setProperty") | ||
| protected void restoreSystemProperty(String value, String key) { |
There was a problem hiding this comment.
nit: This kind of util is easy to use wrong, I would suggest to keep it in your test suite. If you want to share this, a test rule might be a better option or a utility that takes a closure so you can guarantee to reset after executing it
mamazzol
added
Team:Core/Infra
elasticsearchmachine
removed
the
needs:triage
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| @Before | ||
| public void setup() throws IOException, UserException { | ||
| userDir = System.getProperty("user.dir"); | ||
| otelMetricsEnabled = System.getProperty(OTEL_METRICS_ENABLED_SYSTEM_PROPERTY); |
There was a problem hiding this comment.
These are JVM-wide properties that could affect other tests that happen to be running at the same time. Is there a precedent for doing this in other tests? If not, I wonder if we ought to consider another approach like settings?
There was a problem hiding this comment.
I managed to avoid having to modify the user.dir prop. The OTEL prop is still modified but it looks safe given that it's used specifically only for these tests.
| import static org.mockito.Mockito.doReturn; | ||
| import static org.mockito.Mockito.mock; | ||
|
|
||
| @SuppressForbidden(reason = "Need to change value of system property to cover all the scenarios") |
There was a problem hiding this comment.
This is too broad I think. We try to scope these @SuppressForbidden annotations to individual methods, and keep those methods very small.
…elocations * upstream/main: (33 commits) Unmute InferenceRestIT and DefaultEndPointsIT (elastic#144217) feat: add keep_alive to async task status (elastic#144010) Add explicit isNoOpUpdate() method to MapperService (elastic#144113) Always attach APM Agent (elastic#144120) Fix random_score nightly tests (elastic#144176) Add nested query checks for disabled sequence numbers (elastic#144185) Return sentinel values from Fetch when sequence numbers are disabled (elastic#144212) [Test] Test peer-recovery with sequence numbers pruning (elastic#144116) Remove `scaled-*` field assertions from mixed cluster downsampling test (elastic#144295) Refactor: Use range syntax in ES|QL exponential histogram tests (elastic#144110) Move resolve aliases to IndexAbstractionOptions (elastic#143953) unmute test (elastic#144299) Fix approximation csvtests (elastic#144233) fix test (elastic#144171) Add int4 vector scoring benchmarks (elastic#144105) Mute org.elasticsearch.xpack.esql.qa.single_node.GenerativeIT test elastic#143023 Mute org.elasticsearch.test.apmintegration.MetricsApmIT testApmIntegration {withOTel=false} elastic#144282 Native cli launcher (elastic#143712) Mute org.elasticsearch.xpack.esql.qa.multi_node.GenerativeIT test elastic#143023 Mute org.elasticsearch.xpack.esql.heap_attack.HeapAttackSubqueryIT testManyRandomKeywordFieldsInSubqueryIntermediateResults elastic#144274 ...
4 participants
Until there is a clean migration away from the APM Agent for both metrics and tracing, always attach the APM Agent so that, if tracing is enabled later on, the agent is present to record.
ES-14013