jerryshao commented on code in PR #10257:
URL: https://github.com/apache/gravitino/pull/10257#discussion_r2893597775
##########
core/src/test/java/org/apache/gravitino/job/TestBuiltInJobTemplateEventListener.java:
##########
@@ -486,6 +486,29 @@ public void testReconcileHandlesRegistrationException() {
listener.reconcileBuiltInJobTemplates(metalakeName, builtInTemplates);
}
+ @Test
+ public void testReconcileBuiltInJobTemplatesUpdateUsesExistingIdentifier()
throws IOException {
+ String metalakeName = "test_metalake";
+ Map<String, JobTemplate> builtInTemplates = new HashMap<>();
+
+ ShellJobTemplate renamedTemplate =
+ ShellJobTemplate.builder()
+ .withName("builtin-renamed")
+ .withComment("updated")
+ .withExecutable("/bin/echo")
+ .withCustomFields(Collections.singletonMap("version", "v2"))
+ .build();
+ builtInTemplates.put("builtin-existing", renamedTemplate);
+
+ JobTemplateEntity existingEntity =
createJobTemplateEntity("builtin-existing", "v1");
+ entityStore.put(existingEntity, false);
+
+ when(jobManager.listJobTemplates(metalakeName))
+ .thenReturn(Collections.singletonList(existingEntity));
+
+ listener.reconcileBuiltInJobTemplates(metalakeName, builtInTemplates);
+ }
+
private void createMetalake(String name, boolean inUse) throws IOException {
Map<String, String> props = new HashMap<>();
Review Comment:
**Unreachable test scenario** — `loadBuiltInJobTemplates()` always populates
the map as `builtInTemplates.put(template.name(), template)`, so the map key
always equals `newTemplate.name()`. The scenario tested here — where the map
key (`"builtin-existing"`) differs from `newTemplate.name()`
(`"builtin-renamed"`) — cannot occur through the real call path.
Additionally, the test has no explicit assertions. A reader cannot tell from
the test body what the expected outcome is; it simply passes if no exception is
thrown. Consider:
1. Wrapping the call in `Assertions.assertDoesNotThrow(...)` to document the
intent.
2. Verifying in `entityStore` that the entity was actually updated (similar
to `testReconcileBuiltInJobTemplatesUpdate` which checks `version == "v2"`).
3. Using a realistic scenario (same name in map key and template) and
commenting why `existing.name()` is preferred.
--
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]
