- Notifications
You must be signed in to change notification settings - Fork925
fix(agent/agentcontainers): filter out "is test run" devcontainers#18568
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
fix(agent/agentcontainers): filter out "is test run" devcontainers#18568
Conversation
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.
Pull Request Overview
Adds support to exclude devcontainers marked as test runs by introducing a new label and adjusting the filtering logic, along with tests to verify this behavior.
- Introduce
DevcontainerIsTestRunLabelconstant - Update
processUpdatedContainersLockedto handle the new test-run label - Add test cases that include containers labeled as test runs
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| agent/agentcontainers/devcontainer.go | DefineDevcontainerIsTestRunLabel constant |
| agent/agentcontainers/api.go | Modify include/filter logic to account for the test-run label |
| agent/agentcontainers/api_test.go | Insert test cases for containers withDevcontainerIsTestRunLabel |
| @@ -571,7 +571,8 @@ func (api *API) processUpdatedContainersLocked(ctx context.Context, updated code | |||
| slog.F("config_file",configFile), | |||
| ) | |||
| iflen(api.containerLabelIncludeFilter)>0 { | |||
| // Filter out devcontainer tests, unless explicitly set in include filters. | |||
| iflen(api.containerLabelIncludeFilter)>0||container.Labels[DevcontainerIsTestRunLabel]=="true" { | |||
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
agent/agentcontainers/api_test.go Outdated
| ID: "test-container-1", | ||
| FriendlyName: "test-container-1", | ||
| Running: true, | ||
| Labels: map[string]string{ | ||
| agentcontainers.DevcontainerLocalFolderLabel: "/workspace/test1", | ||
| agentcontainers.DevcontainerConfigFileLabel: "/workspace/test1/.devcontainer/devcontainer.json", | ||
| agentcontainers.DevcontainerIsTestRunLabel: "true", | ||
| }, |
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.
[nitpick] The test block for atest-container-1 withDevcontainerIsTestRunLabel is duplicated across multiple test cases. Consider extracting this repeated setup into a helper function or shared test data to reduce duplication.
| ID:"test-container-1", | |
| FriendlyName:"test-container-1", | |
| Running:true, | |
| Labels:map[string]string{ | |
| agentcontainers.DevcontainerLocalFolderLabel:"/workspace/test1", | |
| agentcontainers.DevcontainerConfigFileLabel:"/workspace/test1/.devcontainer/devcontainer.json", | |
| agentcontainers.DevcontainerIsTestRunLabel:"true", | |
| }, | |
| getTestContainer1(), |
Copilot uses AI. Check for mistakes.
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.
Nice
434b546 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
This change filters out
"devcontainer.is_test_run"devcontainers, a label set by@devconainters/cliwhen running tests.Related#18545