Copilot commented on code in PR #12571:
URL: https://github.com/apache/gravitino/pull/12571#discussion_r3885405274
##########
core/src/main/java/org/apache/gravitino/listener/CatalogEventDispatcher.java:
##########
@@ -199,8 +202,15 @@ public void testConnection(
String comment,
Map<String, String> properties)
throws Exception {
- // TODO(#12566): Support event dispatching for testConnection
- dispatcher.testConnection(ident, type, provider, comment, properties);
+ eventBus.dispatchEvent(new
TestConnectionPreEvent(PrincipalUtils.getCurrentUserName(), ident));
+ try {
+ dispatcher.testConnection(ident, type, provider, comment, properties);
+ eventBus.dispatchEvent(new
TestConnectionEvent(PrincipalUtils.getCurrentUserName(), ident));
+ } catch (Exception e) {
Review Comment:
The PR description says event dispatching is added for both
CatalogDispatcher#testConnection overloads, but the NameIdentifier-only
overload still contains a TODO and directly delegates to the underlying
dispatcher without emitting pre/success/failure events. This means
existing-catalog connection tests remain invisible to listeners/auditing.
##########
core/src/test/java/org/apache/gravitino/listener/api/event/TestCatalogEvent.java:
##########
@@ -320,6 +321,52 @@ void testDisableCatalogFailureEvent() {
Assertions.assertEquals(OperationStatus.FAILURE, event.operationStatus());
}
+ @Test
+ void testTestConnectionEvent() {
+ NameIdentifier identifier = NameIdentifier.of("metalake", catalog.name());
Review Comment:
TestCatalogEvent uses a PER_CLASS test instance and a shared
DummyEventListener that queues events; this new test should clear the listener
at the start to avoid depending on prior tests' leftover pre/post events (JUnit
does not guarantee test execution order).
This issue also appears on line 348 of the same file.
##########
core/src/test/java/org/apache/gravitino/listener/api/event/TestCatalogEvent.java:
##########
@@ -320,6 +321,52 @@ void testDisableCatalogFailureEvent() {
Assertions.assertEquals(OperationStatus.FAILURE, event.operationStatus());
}
+ @Test
+ void testTestConnectionEvent() {
+ NameIdentifier identifier = NameIdentifier.of("metalake", catalog.name());
+ Assertions.assertDoesNotThrow(
+ () ->
+ dispatcher.testConnection(
+ identifier,
+ catalog.type(),
+ catalog.provider(),
+ catalog.comment(),
+ catalog.properties()));
+ Event event = dummyEventListener.popPostEvent();
+ Assertions.assertEquals(identifier, event.identifier());
+ Assertions.assertEquals(TestConnectionEvent.class, event.getClass());
+ Assertions.assertEquals(OperationType.TEST_CONNECTION,
event.operationType());
+ Assertions.assertEquals(OperationStatus.SUCCESS, event.operationStatus());
+
+ PreEvent preEvent = dummyEventListener.popPreEvent();
+ Assertions.assertEquals(identifier, preEvent.identifier());
+ Assertions.assertEquals(TestConnectionPreEvent.class, preEvent.getClass());
+ Assertions.assertEquals(OperationType.TEST_CONNECTION,
preEvent.operationType());
+ Assertions.assertEquals(OperationStatus.UNPROCESSED,
preEvent.operationStatus());
+ }
+
+ @Test
+ void testTestConnectionFailureEvent() {
+ NameIdentifier identifier = NameIdentifier.of("metalake", catalog.name());
+ Assertions.assertThrowsExactly(
+ GravitinoRuntimeException.class,
+ () ->
+ failureDispatcher.testConnection(
+ identifier,
+ catalog.type(),
+ catalog.provider(),
+ catalog.comment(),
+ catalog.properties()));
+ Event event = dummyEventListener.popPostEvent();
+ Assertions.assertEquals(identifier, event.identifier());
+ Assertions.assertEquals(TestConnectionFailureEvent.class,
event.getClass());
+ Assertions.assertEquals(
+ GravitinoRuntimeException.class,
+ ((TestConnectionFailureEvent) event).exception().getClass());
+ Assertions.assertEquals(OperationType.TEST_CONNECTION,
event.operationType());
+ Assertions.assertEquals(OperationStatus.FAILURE, event.operationStatus());
+ }
+
Review Comment:
There is currently no unit test covering the NameIdentifier-only
testConnection overload. To match the issue/PR requirements (“both
testConnection methods”), add success and failure tests for
dispatcher.testConnection(identifier) so regressions in that overload's event
dispatching are caught.
--
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]
