- Notifications
You must be signed in to change notification settings - Fork1k
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 ourterms of service andprivacy 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
Uh oh!
There was an error while loading.Please reload this page.
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 morehere, 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 withCodeRabbit:
SupportNeed help? Create a ticket on oursupport 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 beInOrder?
@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 to9381e28Compare@johnstcn I have added an integration-style test now. You can run it with |
ddb5b87 intomainUh oh!
There was an error while loading.Please reload this page.
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.