Copilot commented on code in PR #14487:
URL: https://github.com/apache/cloudstack/pull/14487#discussion_r4356109722
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -669,8 +669,8 @@ public Pair<Boolean, Backup> takeBackup(final
VirtualMachine vm, Boolean quiesce
return new Pair<>(false, backupVO);
}
backupVO.setStatus(Backup.Status.Failed);
- backupDao.remove(backupVO.getId());
- return new Pair<>(false, null);
+ backupDao.update(backupVO.getId(), backupVO);
+ return new Pair<>(false, backupVO);
Review Comment:
`backupDao.update(...)` commonly returns a boolean in CloudStack DAOs; if
the update fails, the method will still return a `Failed` backup object even
though the persisted row may not reflect that status. Consider checking the
return value and either throwing/returning an error state (or at least logging)
so callers don’t assume the DB row was successfully updated.
##########
plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java:
##########
@@ -324,8 +324,10 @@ public void takeBackupFailureWithoutCleanupRemovesBackup()
throws AgentUnavailab
Pair<Boolean, Backup> result = nasBackupProvider.takeBackup(vm, false,
false);
Assert.assertFalse(result.first());
- Assert.assertNull(result.second());
- Mockito.verify(backupDao).remove(Mockito.anyLong());
+ Assert.assertNotNull(result.second());
+ Assert.assertEquals(Backup.Status.Failed, result.second().getStatus());
+ Mockito.verify(backupDao).update(Mockito.anyLong(),
Mockito.any(BackupVO.class));
Review Comment:
The test verifies `update(...)` is called, but it doesn’t assert that the
`BackupVO` passed to `update` actually has `Status.Failed`. To prevent
regressions where the returned object is mutated but the persisted object
isn’t, capture the `BackupVO` argument (e.g., with an `ArgumentCaptor`) and
assert its status is `Failed` (and optionally that the ID matches the expected
backup row).
--
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]
