Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

jtmcg
Copy link

@jtmcg jtmcg commented Feb 5, 2025

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
Copy link
Owner

iamazeem commented Feb 6, 2025

@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
Copy link
Author

jtmcg commented Feb 6, 2025

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