Copilot commented on code in PR #9559:
URL: https://github.com/apache/gravitino/pull/9559#discussion_r2646884781
##########
core/src/test/java/org/apache/gravitino/job/TestJobManager.java:
##########
@@ -114,25 +114,30 @@ public static void setUp() throws IllegalAccessException {
JobManager jm = new JobManager(config, entityStore, idGenerator,
jobExecutor);
jobManager = Mockito.spy(jm);
+ // Stop the background schedulers to prevent interference with tests
+ ScheduledExecutorService cleanUpExecutor =
+ (ScheduledExecutorService) FieldUtils.readField(jobManager,
"cleanUpExecutor", true);
+ if (cleanUpExecutor != null) {
+ cleanUpExecutor.shutdownNow();
+ }
+
+ ScheduledExecutorService statusPullExecutor =
+ (ScheduledExecutorService) FieldUtils.readField(jobManager,
"statusPullExecutor", true);
+ if (statusPullExecutor != null) {
+ statusPullExecutor.shutdownNow();
Review Comment:
The immediate shutdown of the schedulers using `shutdownNow()` right after
JobManager initialization could potentially interrupt any scheduled tasks that
were queued during initialization. While this is likely intentional to prevent
background interference, consider using `shutdown()` followed by
`awaitTermination()` with a short timeout to allow graceful completion of any
pending tasks before forcefully interrupting them.
This would be more consistent with proper resource cleanup patterns and
could prevent potential race conditions if any cleanup logic is executing
during the shutdown.
```suggestion
cleanUpExecutor.shutdown();
try {
if (!cleanUpExecutor.awaitTermination(100, TimeUnit.MILLISECONDS)) {
cleanUpExecutor.shutdownNow();
}
} catch (InterruptedException e) {
cleanUpExecutor.shutdownNow();
Thread.currentThread().interrupt();
}
}
ScheduledExecutorService statusPullExecutor =
(ScheduledExecutorService) FieldUtils.readField(jobManager,
"statusPullExecutor", true);
if (statusPullExecutor != null) {
statusPullExecutor.shutdown();
try {
if (!statusPullExecutor.awaitTermination(100,
TimeUnit.MILLISECONDS)) {
statusPullExecutor.shutdownNow();
}
} catch (InterruptedException e) {
statusPullExecutor.shutdownNow();
Thread.currentThread().interrupt();
}
```
##########
core/src/test/java/org/apache/gravitino/job/TestJobManager.java:
##########
@@ -114,25 +114,30 @@ public static void setUp() throws IllegalAccessException {
JobManager jm = new JobManager(config, entityStore, idGenerator,
jobExecutor);
jobManager = Mockito.spy(jm);
+ // Stop the background schedulers to prevent interference with tests
+ ScheduledExecutorService cleanUpExecutor =
+ (ScheduledExecutorService) FieldUtils.readField(jobManager,
"cleanUpExecutor", true);
+ if (cleanUpExecutor != null) {
+ cleanUpExecutor.shutdownNow();
+ }
+
+ ScheduledExecutorService statusPullExecutor =
+ (ScheduledExecutorService) FieldUtils.readField(jobManager,
"statusPullExecutor", true);
+ if (statusPullExecutor != null) {
+ statusPullExecutor.shutdownNow();
+ }
+
mockedMetalake = mockStatic(MetalakeManager.class);
}
- @AfterAll
- public static void tearDown() throws Exception {
+ @AfterEach
+ public void tearDown() throws Exception {
// Clean up resources if necessary
jobManager.close();
FileUtils.deleteDirectory(new File(testStagingDir));
mockedMetalake.close();
Review Comment:
The removal of the reset() method that called `mockedMetalake.reset()`,
`Mockito.reset(entityStore)`, and `Mockito.reset(jobManager)` means that mock
state is no longer being cleared between tests. While the conversion to
@BeforeEach/@AfterEach provides fresh instances, the mocks created in setUp may
retain state from previous test executions within the same JUnit test class
lifecycle.
Consider adding mock resets in the tearDown method to ensure complete
isolation between tests, especially since these mocks are set up fresh in
setUp. This would prevent any potential test interference from accumulated mock
invocations or stubbing configurations.
```suggestion
// Reset mocks to ensure test isolation
if (mockedMetalake != null) {
mockedMetalake.reset();
}
Mockito.reset(entityStore, jobManager);
// Clean up resources if necessary
jobManager.close();
FileUtils.deleteDirectory(new File(testStagingDir));
if (mockedMetalake != null) {
mockedMetalake.close();
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
