add test cases for multiple vm #287
Conversation
WalkthroughNew end-to-end test file for multi-VM initialization and lifecycle management in Macadam CLI. Tests VM creation, listing, starting, and SSH access verification across multiple VMs. Also adds a Changes
Sequence DiagramsequenceDiagram
participant Test
participant CLI as Macadam CLI
participant VMs as VM System
Test->>CLI: List VMs (BeforeEach)
CLI->>VMs: Query VMs
VMs-->>CLI: 0 VMs (JSON)
CLI-->>Test: Success
rect rgb(200, 220, 255)
Note over Test,VMs: Test Execution
Test->>CLI: Init vm1 (CentOS)
CLI->>VMs: Create vm1
VMs-->>CLI: Success
Test->>CLI: List VMs
CLI-->>Test: 1 VM
Test->>CLI: Start vm1
CLI->>VMs: Start vm1
VMs-->>CLI: Success
Test->>CLI: SSH vm1
CLI->>VMs: SSH connection
VMs-->>CLI: User: "core"
Test->>CLI: Init vm2 (CentOS)
CLI->>VMs: Create vm2
VMs-->>CLI: Success
Test->>CLI: Start vm2
CLI->>VMs: Start vm2
VMs-->>CLI: Success
Test->>CLI: SSH vm2
CLI->>VMs: SSH connection
VMs-->>CLI: User: "core"
Test->>CLI: SSH vm1 (verify still running)
CLI->>VMs: SSH connection
VMs-->>CLI: User: "core"
end
rect rgb(220, 200, 220)
Note over Test,VMs: Cleanup (AfterEach)
Test->>CLI: List all VMs
CLI-->>Test: 2 VMs
Test->>CLI: Stop & Remove all VMs
CLI->>VMs: Remove vm1, vm2
VMs-->>CLI: Success
Test->>CLI: List VMs (verify empty)
CLI-->>Test: 0 VMs
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/multipleVM_test.go (1)
32-33: Remove unnecessary type conversion.The
string()conversion is unnecessary sincem.Nameis already declared asstringin theListReporterstruct.Apply this diff to simplify the code:
- stopCmd := "stop " + string(m.Name) - rmCmd := "rm -f " + string(m.Name) + stopCmd := "stop " + m.Name + rmCmd := "rm -f " + m.Name
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/e2e/multipleVM_test.go(1 hunks)test/e2e/vm_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/e2e/multipleVM_test.go (2)
vendor/github.com/onsi/ginkgo/v2/core_dsl.go (4)
Describe(514-516)BeforeEach(726-728)AfterEach(754-756)It(573-575)vendor/github.com/onsi/gomega/gomega_dsl.go (1)
Expect(220-223)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (windows-latest)
- GitHub Check: lint
- GitHub Check: test (macos-13)
- GitHub Check: e2e (ubuntu-24.04)
🔇 Additional comments (4)
test/e2e/vm_test.go (1)
14-14: LGTM! Name field enables multi-VM test scenarios.The addition of the
Namefield to theListReporterstruct is appropriate and necessary for identifying individual VMs in the new multi-VM test cases. The field is correctly used intest/e2e/multipleVM_test.gofor cleanup operations.test/e2e/multipleVM_test.go (3)
55-111: Excellent multi-VM test coverage.The test case comprehensively validates multi-VM functionality including initialization with names, listing verification at each stage, concurrent VM operation, and SSH access to both VMs. The final check (lines 107-110) confirming vm1 remains accessible after vm2 is started is particularly valuable for verifying concurrent multi-VM operation.
15-22: Dismiss review comment; code is correct.The referenced code is correct. Package-level variables
machineResponsesanderrare properly declared intest/e2e/setup_test.go(lines 19-20). The snippet showserr := json.Unmarshal(...)using the short declaration operator as expected in Go, contradicting the review comment's claim that it lacks:=.Likely an incorrect or invalid review comment.
12-12: Remove the review comment - it is based on incorrect analysis.The
imagevariable is properly declared at package level intest/e2e/setup_test.goline 21 and initialized inBeforeSuite()(lines 31-33). In Go, package-level variables declared in one file are accessible to all files in the same package. The commented-out declaration inmultipleVM_test.goline 12 is redundant but causes no errors—the variable is accessible fromsetup_test.gowhere it's properly initialized before tests run.Likely an incorrect or invalid review comment.
| // check the list command returns one item | ||
| session = macadamTest.Macadam([]string{"list", "--format", "json"}) | ||
| session.WaitWithDefaultTimeout() | ||
| Expect(session).Should(gexec.Exit(0)) | ||
| err = json.Unmarshal(session.Out.Contents(), &machineResponses) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(len(machineResponses)).Should(Equal(2)) |
There was a problem hiding this comment.
Fix inaccurate comment.
The comment on line 86 states "returns one item" but the expectation on line 92 correctly checks for 2 items (vm1 and vm2).
Apply this diff to fix the comment:
- // check the list command returns one item
+ // check the list command returns two items
session = macadamTest.Macadam([]string{"list", "--format", "json"})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // check the list command returns one item | |
| session = macadamTest.Macadam([]string{"list", "--format", "json"}) | |
| session.WaitWithDefaultTimeout() | |
| Expect(session).Should(gexec.Exit(0)) | |
| err = json.Unmarshal(session.Out.Contents(), &machineResponses) | |
| Expect(err).NotTo(HaveOccurred()) | |
| Expect(len(machineResponses)).Should(Equal(2)) | |
| // check the list command returns two items | |
| session = macadamTest.Macadam([]string{"list", "--format", "json"}) | |
| session.WaitWithDefaultTimeout() | |
| Expect(session).Should(gexec.Exit(0)) | |
| err = json.Unmarshal(session.Out.Contents(), &machineResponses) | |
| Expect(err).NotTo(HaveOccurred()) | |
| Expect(len(machineResponses)).Should(Equal(2)) |
🤖 Prompt for AI Agents
In test/e2e/multipleVM_test.go around lines 86 to 92, the inline comment
incorrectly says "returns one item" while the test asserts
Expect(len(machineResponses)).Should(Equal(2)); update the comment to accurately
reflect that the list command should return two items (vm1 and vm2) or remove
the comment; ensure the comment matches the expectation and test intent.
lstocchi
left a comment
lstocchi
left a comment
There was a problem hiding this comment.
As you're reusing the Before/After Suite added in setup_test.go I would move that common logic in common_test.go. It would be more logical from my pov for someone who look at the tests.
Setup_test.go could be misinterpreted ... it could refer to the tests related to setting up (init) a machine (which is what I thought in the prev PR). Now I see that it could also be interpreted like this file contains all code that setup things needed for running tests, but then I would separate the common before/after suite with the actual init command test logic. As we already have common_test.go we could use that instead.
| "github.com/onsi/gomega/gexec" | ||
| ) | ||
|
|
||
| //var image string |
| BeforeEach(func() { | ||
| session := macadamTest.Macadam([]string{"list", "--format", "json"}) | ||
| session.WaitWithDefaultTimeout() | ||
| Expect(session).Should(gexec.Exit(0)) | ||
| err := json.Unmarshal(session.Out.Contents(), &machineResponses) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(len(machineResponses)).Should(Equal(0)) | ||
| }) |
There was a problem hiding this comment.
this is the same code as in setup_test.go, could this be moved in the common_test.go?
| AfterEach(func() { | ||
| session := macadamTest.Macadam([]string{"list", "--format", "json"}) | ||
| session.WaitWithDefaultTimeout() | ||
| Expect(session).Should(gexec.Exit(0)) | ||
| err = json.Unmarshal(session.Out.Contents(), &machineResponses) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| for _, m := range machineResponses { | ||
| stopCmd := "stop " + string(m.Name) | ||
| rmCmd := "rm -f " + string(m.Name) | ||
|
|
||
| // stop the CentOS VM | ||
| session = macadamTest.Macadam(strings.Fields(stopCmd)) | ||
| session.WaitWithDefaultTimeout() | ||
| Expect(session).Should(gexec.Exit(0)) | ||
| Expect(session.OutputToString()).Should(ContainSubstring("stopped successfully")) | ||
|
|
||
| // rm the CentOS VM and verify that "list" does not return any vm | ||
| session = macadamTest.Macadam(strings.Fields(rmCmd)) | ||
| session.WaitWithDefaultTimeout() | ||
| Expect(session).Should(gexec.Exit(0)) | ||
| } | ||
|
|
||
| session = macadamTest.Macadam([]string{"list", "--format", "json"}) | ||
| session.WaitWithDefaultTimeout() | ||
| Expect(session).Should(gexec.Exit(0)) | ||
| err = json.Unmarshal(session.Out.Contents(), &machineResponses) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| Expect(len(machineResponses)).Should(Equal(0)) | ||
| }) |
There was a problem hiding this comment.
same as above. This is very similar to the one in setup_test.go, maybe we could unify them in common_test.go so to not repeat it in every file?
| // ssh into the VM1 and prints user | ||
| session = macadamTest.Macadam([]string{"ssh", "vm1", "whoami"}) | ||
| session.WaitWithDefaultTimeout() | ||
| Expect(session).Should(gexec.Exit(0)) | ||
| Expect(session.OutputToString()).Should(Equal("core")) |
There was a problem hiding this comment.
this is also done at the end of the file. Is it a duplicate to be removed?
2 participants
Summary by CodeRabbit