Skip to content

Add test cases for succeed on no cache and api errors for --all - #1

Merged
iamazeem merged 1 commit into
iamazeem:9897-gh-cache-delete-exit-codefrom
cli:jtmcg/TestDeleteRun-tests
Feb 6, 2025
Merged

iamazeem merged 1 commit into
iamazeem:9897-gh-cache-delete-exit-codefrom
cli:jtmcg/TestDeleteRun-tests

Conversation

@jtmcg

@jtmcg jtmcg commented Feb 5, 2025

Copy link
Copy Markdown

Changes

Added unit tests to TestDeleteRun to cover the following scenarios:

  1. Attempt to delete all caches but the api errors
  2. Command failure when attempting to delete caches when there are none
  3. TTY command success when attempting to delete caches when there are none but the --succeed-on-no-cache flag is set
  4. Non-TTY command success when attempting to delete caches when there are none but the --succeed-on-no-cache flag is set

Note

The first test is in response to this comment on the original PR. I figured if it was enough to cause confusion before, we might as well write a test to cover it here.

The last test is currently failing and will need to be fixed before the original PR can be merged.

@iamazeem

iamazeem commented Feb 6, 2025

Copy link
Copy Markdown
Owner

@jtmcg: Thank you for adding tests! 🥇

Checked this out locally.
Updated tests and verified.
Passing now.

Here's the git diff (removed unused Identifier option):

index a9ec88de..57ecb938 100644
--- a/pkg/cmd/cache/delete/delete_test.go
+++ b/pkg/cmd/cache/delete/delete_test.go
@@ -217,7 +217,7 @@ func TestDeleteRun(t *testing.T) {
                },
                {
                        name: "no caches to delete when deleting all",
-                       opts: DeleteOptions{Identifier: "123", DeleteAll: true},
+                       opts: DeleteOptions{DeleteAll: true},
                        stubs: func(reg *httpmock.Registry) {
                                reg.Register(
                                        httpmock.REST("GET", "repos/OWNER/REPO/actions/caches"),
@@ -233,7 +233,7 @@ func TestDeleteRun(t *testing.T) {
                },
                {
                        name: "no caches to delete when deleting all but succeed on no cache tty",
-                       opts: DeleteOptions{Identifier: "123", DeleteAll: true, SucceedOnNoCaches: true},
+                       opts: DeleteOptions{DeleteAll: true, SucceedOnNoCaches: true},
                        stubs: func(reg *httpmock.Registry) {
                                reg.Register(
                                        httpmock.REST("GET", "repos/OWNER/REPO/actions/caches"),
@@ -249,7 +249,7 @@ func TestDeleteRun(t *testing.T) {
                },
                {
                        name: "no caches to delete when deleting all but succeed on no cache non-tty",
-                       opts: DeleteOptions{Identifier: "123", DeleteAll: true, SucceedOnNoCaches: true},
+                       opts: DeleteOptions{DeleteAll: true, SucceedOnNoCaches: true},
                        stubs: func(reg *httpmock.Registry) {
                                reg.Register(
                                        httpmock.REST("GET", "repos/OWNER/REPO/actions/caches"),
@@ -261,7 +261,7 @@ func TestDeleteRun(t *testing.T) {
                        },
                        tty:        false,
                        wantErr:    false,
-                       wantStdout: "",
+                       wantStdout: "✓ No caches to delete\n",
                },
        }

@jtmcg

jtmcg commented Feb 6, 2025

Copy link
Copy Markdown
Author

Great! You'll either have to merge this yourself (I don't have permissions) or update the existing PR to contain these tests. This will merge directly into that PR's branch, so it should be quite easy

@iamazeem
iamazeem merged commit 2f0f387 into iamazeem:9897-gh-cache-delete-exit-code Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants