From 7b3ba6691215e91fe532c038daf73449b0d4e2a1 Mon Sep 17 00:00:00 2001 From: Niels Bauman Date: Sat, 6 Sep 2025 18:37:00 -0300 Subject: [PATCH 1/2] Fix leaking tests in `HealthPeriodicLoggerTests` At least one (`testClosingWhenRunInProgress`) test, but likely more, were leaking into other tests in `HealthPeriodicLoggerTests`. By triggering the health logger asynchronously in a separate thread, the actual health log would be logged in a subsequent test. `testOutputModeNoLogging` asserts that nothing is logged, so this leaking log would result in a test failure. We simply need to wait for the separate thread to finish before proceeding to the next test. Fixes #134200 --- muted-tests.yml | 3 -- .../health/HealthPeriodicLoggerTests.java | 46 +++++++++++-------- 2 files changed, 26 insertions(+), 23 deletions(-) diff --git a/muted-tests.yml b/muted-tests.yml index 9911717fde45f..e5d54a24b7a85 100644 --- a/muted-tests.yml +++ b/muted-tests.yml @@ -510,9 +510,6 @@ tests: - class: org.elasticsearch.xpack.esql.qa.single_node.GenerativeForkIT method: test {csv-spec:inlinestats.MultiIndexInlinestatsOfMultiTypedField} issue: https://github.com/elastic/elasticsearch/issues/133973 -- class: org.elasticsearch.health.HealthPeriodicLoggerTests - method: testOutputModeNoLogging - issue: https://github.com/elastic/elasticsearch/issues/134200 - class: org.elasticsearch.xpack.esql.qa.single_node.GenerativeForkIT method: test {csv-spec:spatial_shapes.ConvertFromStringParseError} issue: https://github.com/elastic/elasticsearch/issues/134254 diff --git a/server/src/test/java/org/elasticsearch/health/HealthPeriodicLoggerTests.java b/server/src/test/java/org/elasticsearch/health/HealthPeriodicLoggerTests.java index 43d8a74395f1a..c9b0f312f19be 100644 --- a/server/src/test/java/org/elasticsearch/health/HealthPeriodicLoggerTests.java +++ b/server/src/test/java/org/elasticsearch/health/HealthPeriodicLoggerTests.java @@ -436,16 +436,14 @@ public void testTryToLogHealthConcurrencyControlWithResults() throws Exception { SchedulerEngine.Event event = new SchedulerEngine.Event(HealthPeriodicLogger.HEALTH_PERIODIC_LOGGER_JOB_NAME, 0, 0); // run it once, verify getHealth is called - { - Thread logHealthThread = new Thread(() -> testHealthPeriodicLogger.triggered(event)); - logHealthThread.start(); - // We wait to verify that the triggered even is in progress, then we block, so it will rename in progress - assertBusy(() -> assertThat(getHealthCalled.get(), equalTo(1))); - // We try to log again while it's in progress, we expect this run to be skipped - assertFalse(testHealthPeriodicLogger.tryToLogHealth()); - // Unblock the first execution - waitForSecondRun.countDown(); - } + Thread logHealthThread = new Thread(() -> testHealthPeriodicLogger.triggered(event)); + logHealthThread.start(); + // We wait to verify that the triggered even is in progress, then we block, so it will rename in progress + assertBusy(() -> assertThat(getHealthCalled.get(), equalTo(1))); + // We try to log again while it's in progress, we expect this run to be skipped + assertFalse(testHealthPeriodicLogger.tryToLogHealth()); + // Unblock the first execution + waitForSecondRun.countDown(); // run it again, verify getHealth is called, because we are calling the results listener { @@ -453,6 +451,9 @@ public void testTryToLogHealthConcurrencyControlWithResults() throws Exception { testHealthPeriodicLogger.triggered(event); assertBusy(() -> assertThat(getHealthCalled.get(), equalTo(2))); } + + // Wait for the log thread to finish, to ensure it logs during this test and doesn't pollute other tests. + logHealthThread.join(); } public void testTryToLogHealthConcurrencyControl() throws Exception { @@ -485,11 +486,9 @@ public void testTryToLogHealthConcurrencyControl() throws Exception { SchedulerEngine.Event event = new SchedulerEngine.Event(HealthPeriodicLogger.HEALTH_PERIODIC_LOGGER_JOB_NAME, 0, 0); // call it and verify that getHealth is called - { - Thread logHealthThread = new Thread(() -> testHealthPeriodicLogger.triggered(event)); - logHealthThread.start(); - assertBusy(() -> assertThat(getHealthCalled.get(), equalTo(1))); - } + Thread logHealthThread = new Thread(() -> testHealthPeriodicLogger.triggered(event)); + logHealthThread.start(); + assertBusy(() -> assertThat(getHealthCalled.get(), equalTo(1))); // run it again, verify that it's skipped because the other one is in progress { @@ -504,6 +503,9 @@ public void testTryToLogHealthConcurrencyControl() throws Exception { testHealthPeriodicLogger.triggered(event); assertBusy(() -> assertThat(getHealthCalled.get(), equalTo(2))); } + + // Wait for the log thread to finish, to ensure it logs during this test and doesn't pollute other tests. + logHealthThread.join(); } public void testTryToLogHealthConcurrencyControlWithException() throws Exception { @@ -609,11 +611,12 @@ public void testClosingWhenRunInProgress() throws Exception { SchedulerEngine.Event event = new SchedulerEngine.Event(HealthPeriodicLogger.HEALTH_PERIODIC_LOGGER_JOB_NAME, 0, 0); // call it and verify that getHealth is called - { - Thread logHealthThread = new Thread(() -> testHealthPeriodicLogger.triggered(event)); - logHealthThread.start(); - assertBusy(() -> assertTrue(testHealthPeriodicLogger.currentlyRunning())); - } + Thread logHealthThread = new Thread(() -> { + testHealthPeriodicLogger.triggered(event); + logger.info("HALLO"); + }); + logHealthThread.start(); + assertBusy(() -> assertTrue(testHealthPeriodicLogger.currentlyRunning())); // stop and close it { @@ -626,6 +629,9 @@ public void testClosingWhenRunInProgress() throws Exception { waitForCloseToBeTriggered.countDown(); assertBusy(() -> assertEquals(Lifecycle.State.CLOSED, testHealthPeriodicLogger.lifecycleState())); } + + // Wait for the log thread to finish, to ensure it logs during this test and doesn't pollute other tests. + logHealthThread.join(); } } From 623f28a209f5db50d2c0d463a97b0cefe9532ffe Mon Sep 17 00:00:00 2001 From: Niels Bauman Date: Mon, 8 Sep 2025 09:55:40 -0300 Subject: [PATCH 2/2] Remove debug log --- .../org/elasticsearch/health/HealthPeriodicLoggerTests.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/health/HealthPeriodicLoggerTests.java b/server/src/test/java/org/elasticsearch/health/HealthPeriodicLoggerTests.java index c9b0f312f19be..6cd73d1c27bdc 100644 --- a/server/src/test/java/org/elasticsearch/health/HealthPeriodicLoggerTests.java +++ b/server/src/test/java/org/elasticsearch/health/HealthPeriodicLoggerTests.java @@ -611,10 +611,7 @@ public void testClosingWhenRunInProgress() throws Exception { SchedulerEngine.Event event = new SchedulerEngine.Event(HealthPeriodicLogger.HEALTH_PERIODIC_LOGGER_JOB_NAME, 0, 0); // call it and verify that getHealth is called - Thread logHealthThread = new Thread(() -> { - testHealthPeriodicLogger.triggered(event); - logger.info("HALLO"); - }); + Thread logHealthThread = new Thread(() -> testHealthPeriodicLogger.triggered(event)); logHealthThread.start(); assertBusy(() -> assertTrue(testHealthPeriodicLogger.currentlyRunning()));