-
Notifications
You must be signed in to change notification settings - Fork 956
chore(agent/agentcontainers): test current prebuilds integration #19074
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
chore(agent/agentcontainers): test current prebuilds integration #19074
Conversation
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughA new integration-style test, Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🔇 Additional comments (1)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
@coderabbitai generate sequence diagram |
Sequence Diagram for PRsequenceDiagram
participant Test as TestDevcontainerPrebuildSupport
participant API as agentcontainers.API
participant MockContainer as Mock Container
participant MockAgent as Mock Agent
participant MockEnv as Mock Environment
Note over Test: Setup Phase
Test->>API: Initialize API with mocks
Test->>MockContainer: Setup container mock expectations
Test->>MockAgent: Setup agent mock expectations
Note over Test: Prebuild Phase Testing
Test->>API: Create prebuild workspace
API->>MockContainer: Create devcontainer for prebuild
MockContainer-->>API: Return container instance
API->>MockEnv: Set prebuild environment variables
MockEnv-->>API: Environment configured
API-->>Test: Prebuild workspace created
Note over Test: Validation Phase 1
Test->>API: Verify prebuild environment variables
API-->>Test: Environment variables correct
Test->>MockContainer: Verify container creation
MockContainer-->>Test: Container created as expected
Note over Test: Transition Phase Testing
Test->>API: Claim prebuild workspace
API->>MockContainer: Reuse existing container
MockContainer-->>API: Container reused successfully
API->>MockAgent: Create agent for claimed workspace
MockAgent-->>API: Agent created
API->>MockEnv: Update environment for claimed state
MockEnv-->>API: Environment updated
API-->>Test: Workspace claimed successfully
Note over Test: Validation Phase 2
Test->>API: Verify container reuse
API-->>Test: Same container instance used
Test->>MockAgent: Verify agent creation
MockAgent-->>Test: Agent created correctly
Test->>API: Verify environment transition
API-->>Test: Environment properly transitioned
Note over Test: Test Complete
Test->>Test: Assert all expectations met
|
✅ Actions performedSequence diagram generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the principle and see definite value in this test, but worry that the amount of mocking being done here decreases the overall assurances we get.
I don't want to block this PR, but I would suggest adding another test with more of an integration-style approach that uses a well-known devcontainer repo and the 'real' Devcontainer CLI. We couldn't run this by default in CI though, which is the huge value of your current approach.
agent/agentcontainers/api_test.go
Outdated
mDCCLI.EXPECT().Up(gomock.Any(), testDC.WorkspaceFolder, testDC.ConfigPath, gomock.Any()). | ||
Return("test-container-id", nil) | ||
|
||
mCCLI.EXPECT().List(gomock.Any()).Return(codersdk.WorkspaceAgentListContainersResponse{ | ||
Containers: []codersdk.WorkspaceAgentContainer{testContainer}, | ||
}, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these also be InOrder
?
@johnstcn Yeah I can do that. I've refactored the existing test to use the I'll make another integration style test (in line with our other docker only tests) as a more thorough test 👍 |
2db96b0
to
9381e28
Compare
@johnstcn I have added an integration-style test now. You can run it with
|
As it turns out, prebuilds + devcontainers appear to already work together. This PR has created a test that simulates a prebuild claim happening to
agentcontainers.API
, to see how we handle it.