From b5389ca4d3809501e6b5d3a2bc4b5d47639cfdc2 Mon Sep 17 00:00:00 2001 From: Bhanu Reddy Date: Thu, 8 Jan 2026 13:21:08 +0530 Subject: [PATCH 1/2] Fixed success count on deletion error instead of just returning 0 --- artifactory/commands/generic/delete.go | 29 +- artifactory/commands/generic/delete_test.go | 307 ++++++++++++++++++++ 2 files changed, 324 insertions(+), 12 deletions(-) create mode 100644 artifactory/commands/generic/delete_test.go diff --git a/artifactory/commands/generic/delete.go b/artifactory/commands/generic/delete.go index 367965dc..babda691 100644 --- a/artifactory/commands/generic/delete.go +++ b/artifactory/commands/generic/delete.go @@ -1,6 +1,8 @@ package generic import ( + "errors" + ioutils "github.com/jfrog/gofrog/io" "github.com/jfrog/jfrog-cli-core/v2/artifactory/utils" "github.com/jfrog/jfrog-cli-core/v2/common/spec" @@ -46,12 +48,13 @@ func (dc *DeleteCommand) Run() (err error) { } } if allowDelete { - var successCount int - var failedCount int - successCount, failedCount, err = dc.DeleteFiles(reader) + successCount, failedCount, deleteErr := dc.DeleteFiles(reader) + // Always set the result counts, even if an error occurred + // This follows the pattern used by move/copy commands result := dc.Result() - result.SetFailCount(failedCount) result.SetSuccessCount(successCount) + result.SetFailCount(failedCount) + err = deleteErr } return } @@ -108,15 +111,17 @@ func (dc *DeleteCommand) DeleteFiles(reader *content.ContentReader) (successCoun if err != nil { return 0, 0, err } - deletedCount, err := servicesManager.DeleteFiles(reader) - if err != nil { - return 0, 0, err - } - length, err := reader.Length() - if err != nil { - return 0, 0, err + // Perform deletion - capture both count and error separately + deletedCount, deleteErr := servicesManager.DeleteFiles(reader) + // Get total length to calculate failed count + length, lengthErr := reader.Length() + if lengthErr != nil { + // If we can't get the length, return what we have with the original error + return deletedCount, 0, errors.Join(deleteErr, lengthErr) } - return deletedCount, length - deletedCount, err + // Return actual counts even when there was an error during deletion + // This follows the pattern used by move/copy commands + return deletedCount, length - deletedCount, deleteErr } func getDeleteParams(f *spec.File) (deleteParams services.DeleteParams, err error) { diff --git a/artifactory/commands/generic/delete_test.go b/artifactory/commands/generic/delete_test.go new file mode 100644 index 00000000..47298a3d --- /dev/null +++ b/artifactory/commands/generic/delete_test.go @@ -0,0 +1,307 @@ +package generic + +import ( + "errors" + "testing" + + "github.com/jfrog/jfrog-cli-core/v2/common/spec" + "github.com/stretchr/testify/assert" +) + +func TestNewDeleteCommand(t *testing.T) { + dc := NewDeleteCommand() + assert.NotNil(t, dc) + assert.Equal(t, "rt_delete", dc.CommandName()) + assert.NotNil(t, dc.Result()) +} + +func TestDeleteCommand_SetThreads(t *testing.T) { + dc := NewDeleteCommand() + dc.SetThreads(5) + assert.Equal(t, 5, dc.Threads()) +} + +func TestDeleteCommand_Threads(t *testing.T) { + dc := NewDeleteCommand() + + // Default threads should be 0 + assert.Equal(t, 0, dc.Threads()) + + // Set and verify threads + dc.SetThreads(10) + assert.Equal(t, 10, dc.Threads()) + + // Chaining should work + result := dc.SetThreads(3) + assert.Equal(t, dc, result) + assert.Equal(t, 3, dc.Threads()) +} + +func TestGetDeleteParams(t *testing.T) { + tests := []struct { + name string + file *spec.File + expectedRecursive bool + }{ + { + name: "Basic pattern with default recursive", + file: &spec.File{ + Pattern: "repo/path/*", + }, + expectedRecursive: true, + }, + { + name: "Pattern with recursive true", + file: &spec.File{ + Pattern: "repo/path/*", + Recursive: "true", + }, + expectedRecursive: true, + }, + { + name: "Pattern with recursive false", + file: &spec.File{ + Pattern: "repo/path/*", + Recursive: "false", + }, + expectedRecursive: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + params, err := getDeleteParams(tt.file) + assert.NoError(t, err) + assert.Equal(t, tt.file.Pattern, params.Pattern) + assert.Equal(t, tt.expectedRecursive, params.Recursive) + }) + } +} + +func TestGetDeleteParams_ExcludeArtifacts(t *testing.T) { + tests := []struct { + name string + excludeArtifacts string + expectedExcludeArtifacts bool + }{ + { + name: "Default exclude artifacts (false)", + excludeArtifacts: "", + expectedExcludeArtifacts: false, + }, + { + name: "Exclude artifacts true", + excludeArtifacts: "true", + expectedExcludeArtifacts: true, + }, + { + name: "Exclude artifacts false", + excludeArtifacts: "false", + expectedExcludeArtifacts: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + file := &spec.File{ + Pattern: "repo/*", + ExcludeArtifacts: tt.excludeArtifacts, + } + params, err := getDeleteParams(file) + assert.NoError(t, err) + assert.Equal(t, tt.expectedExcludeArtifacts, params.ExcludeArtifacts) + }) + } +} + +func TestGetDeleteParams_IncludeDeps(t *testing.T) { + tests := []struct { + name string + includeDeps string + expectedIncludeDeps bool + }{ + { + name: "Default include deps (false)", + includeDeps: "", + expectedIncludeDeps: false, + }, + { + name: "Include deps true", + includeDeps: "true", + expectedIncludeDeps: true, + }, + { + name: "Include deps false", + includeDeps: "false", + expectedIncludeDeps: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + file := &spec.File{ + Pattern: "repo/*", + IncludeDeps: tt.includeDeps, + } + params, err := getDeleteParams(file) + assert.NoError(t, err) + assert.Equal(t, tt.expectedIncludeDeps, params.IncludeDeps) + }) + } +} + +func TestGetDeleteParams_CombinedFlags(t *testing.T) { + file := &spec.File{ + Pattern: "repo/path/**/*.jar", + Recursive: "true", + ExcludeArtifacts: "true", + IncludeDeps: "true", + Exclusions: []string{"*-sources.jar", "*-javadoc.jar"}, + } + + params, err := getDeleteParams(file) + assert.NoError(t, err) + + assert.Equal(t, "repo/path/**/*.jar", params.Pattern) + assert.True(t, params.Recursive) + assert.True(t, params.ExcludeArtifacts) + assert.True(t, params.IncludeDeps) + assert.Len(t, params.Exclusions, 2) +} + +func TestGetDeleteParams_WithExclusions(t *testing.T) { + file := &spec.File{ + Pattern: "repo/*.bin", + Exclusions: []string{ + "*-test.bin", + "*-debug.bin", + "temp/*", + }, + } + + params, err := getDeleteParams(file) + + assert.NoError(t, err) + assert.Len(t, params.Exclusions, 3) + assert.Contains(t, params.Exclusions, "*-test.bin") + assert.Contains(t, params.Exclusions, "*-debug.bin") + assert.Contains(t, params.Exclusions, "temp/*") +} + +func TestGetDeleteParams_EmptyPattern(t *testing.T) { + file := &spec.File{ + Pattern: "", + } + + params, err := getDeleteParams(file) + + assert.NoError(t, err) + assert.Equal(t, "", params.Pattern) +} + +func TestDeleteCommand_ResultInitialized(t *testing.T) { + dc := NewDeleteCommand() + + // Result should be initialized (not nil) + result := dc.Result() + assert.NotNil(t, result) + + // Default counts should be 0 + assert.Equal(t, 0, result.SuccessCount()) + assert.Equal(t, 0, result.FailCount()) +} + +func TestDeleteCommand_GenericCommandEmbedded(t *testing.T) { + dc := NewDeleteCommand() + + // Test GenericCommand methods are accessible + dc.SetDryRun(true) + assert.True(t, dc.DryRun()) + + dc.SetQuiet(true) + assert.True(t, dc.Quiet()) + + dc.SetRetries(3) + assert.Equal(t, 3, dc.Retries()) +} + +// TestErrorsJoin verifies that errors.Join works correctly for combining errors +// This tests the behavior used in DeleteFiles when both delete and length errors occur +func TestErrorsJoin(t *testing.T) { + err1 := errors.New("delete error") + err2 := errors.New("length error") + + combined := errors.Join(err1, err2) + assert.Error(t, combined) + assert.Contains(t, combined.Error(), "delete error") + assert.Contains(t, combined.Error(), "length error") + + // Test with nil errors + combinedWithNil := errors.Join(nil, err2) + assert.Error(t, combinedWithNil) + assert.Contains(t, combinedWithNil.Error(), "length error") + + combinedBothNil := errors.Join(nil, nil) + assert.NoError(t, combinedBothNil) +} + +// TestDeleteFilesCountBehavior documents the expected behavior of count calculations +// deletedCount represents successfully deleted items +// length - deletedCount represents failed items +func TestDeleteFilesCountBehavior(t *testing.T) { + tests := []struct { + name string + deletedCount int + totalLength int + expectedSuccessCount int + expectedFailedCount int + }{ + { + name: "All items deleted successfully", + deletedCount: 10, + totalLength: 10, + expectedSuccessCount: 10, + expectedFailedCount: 0, + }, + { + name: "Some items failed", + deletedCount: 7, + totalLength: 10, + expectedSuccessCount: 7, + expectedFailedCount: 3, + }, + { + name: "All items failed", + deletedCount: 0, + totalLength: 10, + expectedSuccessCount: 0, + expectedFailedCount: 10, + }, + { + name: "No items to delete", + deletedCount: 0, + totalLength: 0, + expectedSuccessCount: 0, + expectedFailedCount: 0, + }, + { + name: "Partial success (404 scenario)", + deletedCount: 5, + totalLength: 8, + expectedSuccessCount: 5, + expectedFailedCount: 3, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // This simulates the count calculation in DeleteFiles + successCount := tt.deletedCount + failedCount := tt.totalLength - tt.deletedCount + + assert.Equal(t, tt.expectedSuccessCount, successCount) + assert.Equal(t, tt.expectedFailedCount, failedCount) + }) + } +} + From 2ab92660e414a20fffe7a15c89542b7107bfcdc4 Mon Sep 17 00:00:00 2001 From: Bhanu Reddy Date: Tue, 13 Jan 2026 05:35:27 +0530 Subject: [PATCH 2/2] Improved readability --- artifactory/commands/generic/delete.go | 4 ++-- artifactory/commands/generic/delete_test.go | 11 +++++------ 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/artifactory/commands/generic/delete.go b/artifactory/commands/generic/delete.go index babda691..5e66dc72 100644 --- a/artifactory/commands/generic/delete.go +++ b/artifactory/commands/generic/delete.go @@ -48,13 +48,13 @@ func (dc *DeleteCommand) Run() (err error) { } } if allowDelete { - successCount, failedCount, deleteErr := dc.DeleteFiles(reader) + var successCount, failedCount int + successCount, failedCount, err = dc.DeleteFiles(reader) // Always set the result counts, even if an error occurred // This follows the pattern used by move/copy commands result := dc.Result() result.SetSuccessCount(successCount) result.SetFailCount(failedCount) - err = deleteErr } return } diff --git a/artifactory/commands/generic/delete_test.go b/artifactory/commands/generic/delete_test.go index 47298a3d..d84de2b0 100644 --- a/artifactory/commands/generic/delete_test.go +++ b/artifactory/commands/generic/delete_test.go @@ -250,11 +250,11 @@ func TestErrorsJoin(t *testing.T) { // length - deletedCount represents failed items func TestDeleteFilesCountBehavior(t *testing.T) { tests := []struct { - name string - deletedCount int - totalLength int - expectedSuccessCount int - expectedFailedCount int + name string + deletedCount int + totalLength int + expectedSuccessCount int + expectedFailedCount int }{ { name: "All items deleted successfully", @@ -304,4 +304,3 @@ func TestDeleteFilesCountBehavior(t *testing.T) { }) } } -