fix(tests): use valid hex instance IDs in delete and status tests#713
fix(tests): use valid hex instance IDs in delete and status tests#713ArangoGutierrez wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
The instance ID validation (instances.go) requires IDs to match
^[0-9a-f]{8}$ or UUID format. Several tests used arbitrary strings
(sshdelete1, test12345678, customdel, etc.) that fail this validation.
Replace all test instance IDs with valid 8-character hex strings.
Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
There was a problem hiding this comment.
Pull request overview
This PR updates CLI unit tests to use instance IDs that comply with Holodeck’s instance ID validation (^[0-9a-f]{8}$ or UUID), preventing failures caused by invalid test data.
Changes:
- Updated
statuscommand tests to use a valid 8-char hex instance ID in the “instance exists” case. - Updated
deletecommand tests to use valid 8-char hex instance IDs for SSH cache filenames/arguments and related assertions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
cmd/cli/status/status_test.go |
Replaces an invalid instance ID with a valid 8-char hex ID in the “instance exists” test YAML and invocation. |
cmd/cli/delete/delete_test.go |
Replaces several invalid SSH instance IDs with valid 8-char hex IDs across single/multi delete tests and assertions. |
You can also share your feedback on Copilot code review. Take the survey.
| // First instance doesn't exist, should fail before processing second | ||
| err = app.Run([]string{"holodeck", "delete", "--cachepath", tempDir, "nonexistent", "sshvalid1"}) | ||
| err = app.Run([]string{"holodeck", "delete", "--cachepath", tempDir, "nonexistent", "a3b4c5d6"}) | ||
| Expect(err).To(HaveOccurred()) | ||
| Expect(err.Error()).To(ContainSubstring("failed to get instance nonexistent")) |
There was a problem hiding this comment.
These scenarios are described as “instance doesn’t exist”, but the test still uses the instance ID "nonexistent", which fails the instance ID format validation (only 8-char hex or UUID). That means the command may fail due to invalid input rather than a missing cache entry, so the test doesn’t cover the intended behavior. Consider switching "nonexistent" to a valid-looking ID (e.g., an 8-char hex string not present in the temp cache dir) and updating the expected error substring accordingly.
| err = app.Run([]string{"holodeck", "delete", "--cachepath", tempDir, "e7f8a9b0", "nonexistent"}) | ||
| Expect(err).To(HaveOccurred()) | ||
| Expect(err.Error()).To(ContainSubstring("failed to get instance nonexistent")) |
There was a problem hiding this comment.
This test uses "nonexistent" as an instance ID, which is not a valid ID format per the instance ID validation (8-char hex or UUID). If the goal is to test the “second instance missing” path, use a valid-format ID that has no cache file so the failure is due to missing instance rather than invalid input.
| err = app.Run([]string{"holodeck", "delete", "--cachepath", tempDir, "e7f8a9b0", "nonexistent"}) | |
| Expect(err).To(HaveOccurred()) | |
| Expect(err.Error()).To(ContainSubstring("failed to get instance nonexistent")) | |
| err = app.Run([]string{"holodeck", "delete", "--cachepath", tempDir, "e7f8a9b0", "deadbeef"}) | |
| Expect(err).To(HaveOccurred()) | |
| Expect(err.Error()).To(ContainSubstring("failed to get instance deadbeef")) |
Summary
cmd/cli/delete(4) andcmd/cli/status(1)sshdelete1,test12345678,customdel, etc.) as instance IDs that don't match the^[0-9a-f]{8}$or UUID validation pattern ininternal/instances/instances.goTest plan
go test ./cmd/cli/delete/...— 9/9 passgo test ./cmd/cli/status/...— 9/9 pass