Experiment cleanup support test cases

[DImuthuUpe]

No description provided.

[Copilot]

Pull request overview

This PR adds test cases to support experiment cleanup functionality. The changes introduce SFTP directory deletion capabilities and test infrastructure to verify the functionality.

• Added new test class SFTPDeleteDirTest.java with test cases for deleting empty and non-empty directories via SFTP
• Implemented recursive directory deletion functionality in SSHJAgentAdaptor
• Renamed field from cleanupStrategy to cleanUpStrategy in ExperimentEntity for consistency
• Removed jmockit test configuration from root pom.xml and added Apache SSHD test dependency

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
pom.xml	Removed jmockit agent configuration from maven-surefire-plugin
airavata-api/pom.xml	Added Apache SSHD SFTP test dependency (version 2.12.1)
airavata-api/src/test/java/org/apache/airavata/helix/SFTPDeleteDirTest.java	New test class for SFTP directory deletion with embedded SSH server setup
airavata-api/src/main/java/org/apache/airavata/registry/core/entities/expcatalog/ExperimentEntity.java	Renamed cleanupStrategy field and getter/setter methods to cleanUpStrategy
airavata-api/src/main/java/org/apache/airavata/helix/adaptor/SSHJAgentAdaptor.java	Added recursive directory deletion implementation (contains duplicate method definition)

Comments suppressed due to low confidence (1)

airavata-api/src/main/java/org/apache/airavata/helix/adaptor/SSHJAgentAdaptor.java:350

• Duplicate method definition detected. The deleteDirectory method is defined twice (lines 299-323 and 326-350). The first implementation supports recursive deletion using deleteDirectoryRecursively, while the second one only calls rmdir which won't work for non-empty directories. The second definition will override the first, breaking the recursive deletion functionality that the tests expect.

    @Override
    public void deleteDirectory(String path) throws AgentException {
        if (path == null || path.trim().isEmpty()) {
            throw new AgentException("Directory path cannot be null or empty");
        }
        SFTPClientWrapper sftpClient = null;
        try {
            sftpClient = sshjClient.newSFTPClientWrapper();
            deleteDirectoryRecursively(sftpClient, path);
        } catch (Exception e) {
            if (e instanceof ConnectionException) {
                Optional.ofNullable(sftpClient).ifPresent(ft -> ft.setErrored(true));
            }
            logger.error("Error while deleting directory {}", path, e);
            throw new AgentException(e);

        } finally {
            Optional.ofNullable(sftpClient).ifPresent(client -> {
                try {
                    client.close();
                } catch (IOException e) {
                    // Ignore
                }
            });
        }
    }

    @Override
    public void deleteDirectory(String path) throws AgentException {
        if (path == null || path.trim().isEmpty()) {
            throw new AgentException("Directory path cannot be null or empty");
        }
        SFTPClientWrapper sftpClient = null;
        try {
            sftpClient = sshjClient.newSFTPClientWrapper();
            sftpClient.rmdir(path);
        } catch (Exception e) {
            if (e instanceof ConnectionException) {
                Optional.ofNullable(sftpClient).ifPresent(ft -> ft.setErrored(true));
            }
            logger.error("Error while deleting directory {}", path, e);
            throw new AgentException(e);

        } finally {
            Optional.ofNullable(sftpClient).ifPresent(client -> {
                try {
                    client.close();
                } catch (IOException e) {
                    // Ignore
                }
            });
        }
    }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

[Copilot]

Path concatenation issue: Using string concatenation with "/" to build paths (e.g., path + "/" + r.getName()) may cause issues on Windows systems or when dealing with paths that already end with a separator. Consider using proper path handling utilities or checking if the path already ends with a separator before concatenating.

[Copilot]

Missing filter for special directory entries. The ls command typically returns "." and ".." entries in directory listings. The recursive delete function doesn't filter these out, which could cause issues or inefficiency. Consider filtering out these entries before processing: for (RemoteResourceInfo r : ls) { if (".".equals(r.getName()) || "..".equals(r.getName())) continue; ... }

Suggested change

	for (RemoteResourceInfo r : ls) {
	for (RemoteResourceInfo r : ls) {
	if (".".equals(r.getName()) || "..".equals(r.getName())) {
	continue;
	}

[Copilot]

[nitpick] Inconsistent naming: The field name has been changed from cleanupStrategy to cleanUpStrategy. While "cleanup" is often written as one word in technical contexts, this change should be consistent with the column name "CLEANUP_STRATEGY" (which uses the single-word form). Consider whether this naming inconsistency between the Java field and database column is intentional.

Suggested change

	public ExperimentCleanupStrategy cleanUpStrategy;
	public ExperimentCleanupStrategy cleanupStrategy;

[Copilot]

Missing assertions in test method. The test deleteNonEmptyDir verifies that a non-empty directory can be deleted but doesn't include any assertions to validate the expected behavior. The test should verify that 'dir1' is actually removed and no longer appears in itemsAfter.

[Copilot]

Resource leak: The SSHJAgentAdaptor instance is created but never properly closed. The adaptor has a destroy() method that should be called to clean up SSH connections. Consider adding adaptor.destroy() in a finally block or use a try-with-resources pattern if the class implements AutoCloseable.

[Copilot]

Resource leak: The SSHJAgentAdaptor instance is created but never properly closed. The adaptor has a destroy() method that should be called to clean up SSH connections. Consider adding adaptor.destroy() in a finally block or use a try-with-resources pattern if the class implements AutoCloseable.