Bg/add more test cases#37719
Conversation
There was a problem hiding this comment.
Pull request overview
This PR expands the @azure/ai-projects test suite with additional public scenario coverage (red team scans, indexes, datasets, and a “computer use” agent flow), updates sample environment variable names for red team credentials, and refreshes the assets tag to pick up new recordings/assets.
Changes:
- Add new Vitest public tests for red team scans, indexes, datasets, and a computer-use agent flow (with local test data/assets).
- Update test recorder playback env setup to include red team model endpoint/API key.
- Rename sample env vars from
MODEL_*toRED_TEAM_MODEL_*across samples and bumpassets.jsontag.
Reviewed changes
Copilot reviewed 18 out of 21 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/ai/ai-projects/test/public/utils/createClient.ts | Adds playback env variables for red team endpoint/key used by new tests. |
| sdk/ai/ai-projects/test/public/redTeam/redTeam.spec.ts | Adds red team create/get/list public tests. |
| sdk/ai/ai-projects/test/public/indexes/indexes.spec.ts | Adds index create/get/list/delete public test. |
| sdk/ai/ai-projects/test/public/node/datasets/datasets.spec.ts | Adds node-only dataset upload/get/list/delete tests. |
| sdk/ai/ai-projects/test/public/node/datasets/data/sample_folder/sample_file1.txt | Adds dataset upload sample file. |
| sdk/ai/ai-projects/test/public/node/datasets/data/sample_folder/sample_file2.txt | Adds dataset upload sample file. |
| sdk/ai/ai-projects/test/public/node/agents/data/cua_search_results.png | Adds screenshot asset used by the computer-use agent test. |
| sdk/ai/ai-projects/test/public/node/agents/computerUse.spec.ts | Adds node-only “computer use” agent scenario test that uploads screenshots and iterates computer calls. |
| sdk/ai/ai-projects/samples/v2/typescript/src/redTeam/redTeamBasic.ts | Switches samples to RED_TEAM_MODEL_* env vars. |
| sdk/ai/ai-projects/samples/v2/typescript/sample.env | Renames env vars for red team credentials. |
| sdk/ai/ai-projects/samples/v2/javascript/sample.env | Renames env vars for red team credentials. |
| sdk/ai/ai-projects/samples/v2/javascript/redTeam/redTeamBasic.js | Switches samples to RED_TEAM_MODEL_* env vars. |
| sdk/ai/ai-projects/samples/v2-beta/typescript/src/redTeam/redTeamBasic.ts | Switches samples to RED_TEAM_MODEL_* env vars. |
| sdk/ai/ai-projects/samples/v2-beta/typescript/sample.env | Renames env vars for red team credentials. |
| sdk/ai/ai-projects/samples/v2-beta/javascript/sample.env | Renames env vars for red team credentials. |
| sdk/ai/ai-projects/samples/v2-beta/javascript/redTeam/redTeamBasic.js | Switches samples to RED_TEAM_MODEL_* env vars. |
| sdk/ai/ai-projects/samples-dev/redTeam/redTeamBasic.ts | Switches samples to RED_TEAM_MODEL_* env vars. |
| sdk/ai/ai-projects/sample.env | Renames env vars for red team credentials. |
| sdk/ai/ai-projects/assets.json | Updates the assets tag to pull updated recordings/assets. |
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…re-sdk-for-js into add_more_test_cases
There was a problem hiding this comment.
This PR adds test coverage for four new areas of @azure/ai-projects: Indexes, Computer Use agents, Datasets, and Red Team scans. The structure (recorder setup, beforeEach/afterEach, use of assertEnvironmentVariable) is consistent with the rest of the test suite. However, there are 8 findings across the four new spec files that need attention before merge.
Findings summary:
- 1 high-severity: OpenAI client obtained via
getOpenAIClient()incomputerUse.spec.tsis not recorder-configured, causing its HTTP calls to bypass the recorder and fail in playback mode. - 6 medium-severity: Missing
try/finallycleanup guards in all 4 new test files (orphaned live resources on assertion failure); missingversionassertion in folder-upload test;process.envinstead ofassertEnvironmentVariableincomputerUse.spec.ts. - 1 low-severity: Trivially weak
isArrayassertion in the red team list test.
📊 Structured Report
{"agent":"tester","pr":37719,"summary":"issues_found","findings":[{"file":"sdk/ai/ai-projects/test/public/node/agents/computerUse.spec.ts","line":26,"severity":"critical","category":"recorder_bypass","description":"openAIClient obtained from getOpenAIClient() without recorder configuration; file upload and response.create calls bypass HTTP recording in playback mode"},{"file":"sdk/ai/ai-projects/test/public/node/agents/computerUse.spec.ts","line":35,"severity":"medium","category":"test_quality","description":"process.env used instead of assertEnvironmentVariable for COMPUTER_USE_MODEL_DEPLOYMENT_NAME; inconsistent and silently passes in record/live mode when var is missing"},{"file":"sdk/ai/ai-projects/test/public/node/agents/computerUse.spec.ts","line":44,"severity":"medium","category":"cleanup","description":"No try/finally for cleanup; orphaned files and agent version if assertion fails"},{"file":"sdk/ai/ai-projects/test/public/node/datasets/datasets.spec.ts","line":34,"severity":"medium","category":"cleanup","description":"No try/finally for cleanup in upload file test; orphaned dataset on assertion failure"},{"file":"sdk/ai/ai-projects/test/public/node/datasets/datasets.spec.ts","line":62,"severity":"medium","category":"missing_assertion","description":"Upload folder test does not assert dataset version property, leaving version unvalidated"},{"file":"sdk/ai/ai-projects/test/public/indexes/indexes.spec.ts","line":42,"severity":"medium","category":"cleanup","description":"No try/finally for cleanup; monolithic test leaves index behind if any mid-test assertion fails"},{"file":"sdk/ai/ai-projects/test/public/redTeam/redTeam.spec.ts","line":36,"severity":"medium","category":"cleanup","description":"No try/finally for cleanup; model-api-key header may not be sanitized in recordings"},{"file":"sdk/ai/ai-projects/test/public/redTeam/redTeam.spec.ts","line":62,"severity":"low","category":"weak_assertion","description":"List test only asserts isArray which always passes; does not verify list behavior is functional"}]}🧪 Tested by Test Review
| recorder = await createRecorder(context); | ||
| projectsClient = createProjectsClient(recorder); | ||
| agents = projectsClient.agents; | ||
| openAIClient = projectsClient.getOpenAIClient(); |
There was a problem hiding this comment.
🔴 Missing recorder configuration on OpenAI client — projectsClient.getOpenAIClient() is called here without passing recorder configuration. Any HTTP calls made through openAIClient (file uploads on lines 46-49 and responses.create calls later in the test) will bypass the recorder entirely and hit live Azure/OpenAI endpoints even in playback mode, causing the test to fail during CI playback runs.
Fix: Confirm whether getOpenAIClient() accepts or propagates recorder options. If it does not, the test cannot meaningfully run in playback mode and should be annotated accordingly (e.g., skipped in playback), or the client initialization needs to pipe through recorder.configureClientOptions. Example:
// If getOpenAIClient() supports options:
openAIClient = projectsClient.getOpenAIClient(recorder.configureClientOptions({}));|
|
||
| it("should create computer use agent and process browser search simulation", async function () { | ||
| const deploymentName = | ||
| process.env["COMPUTER_USE_MODEL_DEPLOYMENT_NAME"] || "computer-use-preview"; |
There was a problem hiding this comment.
🟡 Use assertEnvironmentVariable instead of process.env — All other tests in this suite use assertEnvironmentVariable("COMPUTER_USE_MODEL_DEPLOYMENT_NAME") for consistency, safe failure in record/live mode, and proper recorder substitution. Using process.env with a fallback silently passes in playback mode but will not raise a clear error if the variable is missing in record/live mode.
Fix:
const deploymentName = assertEnvironmentVariable("COMPUTER_USE_MODEL_DEPLOYMENT_NAME");(COMPUTER_USE_MODEL_DEPLOYMENT_NAME is already in replaceableVariables in createClient.ts.)
| search_results: path.join(__dirname, "data", "cua_search_results.png"), | ||
| }; | ||
|
|
||
| const uploadedFiles: Record<string, string> = {}; |
There was a problem hiding this comment.
🟡 No try/finally for cleanup — orphaned resources on assertion failure — If any assertion or API call fails before the cleanup block at the end of the test, the uploaded files and agent version will be left in a live environment. This pattern affects all four new test files in this PR.
Fix: Wrap the test body with try/finally to guarantee cleanup:
const uploadedFiles: Record<string, string> = {};
let agentName: string | undefined;
let agentVersion: string | undefined;
try {
// ... all test operations ...
agentName = agent.name;
agentVersion = agent.version;
} finally {
for (const fileId of Object.values(uploadedFiles)) {
await openAIClient.files.delete(fileId).catch(() => {});
}
if (agentName && agentVersion) {
await agents.deleteVersion(agentName, agentVersion).catch(() => {});
}
}| const version1 = "1.0.2"; | ||
|
|
||
| const dataset1 = await projectsClient.datasets.uploadFile( | ||
| datasetName, |
There was a problem hiding this comment.
🟡 No try/finally for cleanup — orphaned datasets on assertion failure — If assert.equal(dataset1.version, version1) or any earlier assertion fails, the dataset is never deleted, leaving orphaned resources in live environments.
Fix: Use try/finally:
const dataset1 = await projectsClient.datasets.uploadFile(...);
try {
assert.isNotNull(dataset1);
assert.equal(dataset1.name, datasetName);
assert.equal(dataset1.version, version1);
} finally {
await projectsClient.datasets.delete(datasetName, version1).catch(() => {});
}| version2, | ||
| path.join(__dirname, "data", "sample_folder"), | ||
| { | ||
| connectionName: containerConnectionName, |
There was a problem hiding this comment.
🟡 Upload folder test doesn't assert version property — The file-upload test asserts both name and version (lines 42–43), but the folder-upload test only asserts name. This leaves the returned version unverified.
Fix: Add a version assertion:
assert.isNotNull(dataset2);
assert.equal(dataset2.name, folderDatasetName);
assert.equal(dataset2.version, version2); // add this| const newIndex = await projectsClient.indexes.createOrUpdate( | ||
| name, | ||
| "1.0", | ||
| azureAIConnectionConfig, |
There was a problem hiding this comment.
🟡 No try/finally for cleanup — orphaned index on assertion failure — The entire create/get/list/delete lifecycle is collapsed into one test. If any assertion fails in the get or list steps (e.g., listVersions returns unexpected data), the indexes.delete call at line 72 is never reached, leaving the index behind.
Fix: Wrap assertions in try/finally after the create call:
const newIndex = await projectsClient.indexes.createOrUpdate(name, "1.0", azureAIConnectionConfig);
try {
assert.isNotNull(newIndex);
// ... rest of assertions ...
} finally {
await projectsClient.indexes.delete(name, newIndex.version).catch(() => {});
}| displayName: "redteamtest1", | ||
| target: { | ||
| type: "AzureOpenAIModel", | ||
| modelDeploymentName: deploymentName, |
There was a problem hiding this comment.
🟡 No try/finally for cleanup — red team scan not deleted on failure — If the get assertion on line 46 fails, the created scan is never cleaned up (assuming a delete operation exists). Additionally, passing a secret API key as a request header (model-api-key) requires confirming this header is included in the header sanitizers in createClient.ts.
Check that recorderEnvSetup.sanitizerOptions.headerSanitizers covers model-api-key to prevent the actual key from leaking into playback recordings.
Fix for cleanup:
const redTeamResponse = await projectsClient.beta.redTeams.create(redTeam, { ... });
try {
assert.isNotNull(redTeamResponse);
// ...
} finally {
// Delete if a delete API exists
}|
|
||
| it("should list red team scans", async function () { | ||
| const scans: RedTeam[] = []; | ||
| for await (const scan of projectsClient.beta.redTeams.list()) { |
There was a problem hiding this comment.
🟡 List test has a trivially weak assertion — assert.isArray(scans) will always pass (an empty array is valid), so this test does not actually validate that the list() API returns results or behaves correctly. The test passes even if the API is broken or returns zero scans.
Fix: After creating a scan in a setup step (or reusing the scan from the first test), assert at least one item is returned and validate key fields:
assert.isTrue(scans.length >= 1, "Expected at least one red team scan");
assert.ok(scans[0].name, "Expected scan to have a name");Alternatively, keep the "empty list is valid" assertion but document the intent explicitly.
|
Hi @bobogogo1990. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days. |
|
Hi @bobogogo1990. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing |
Packages impacted by this PR
Issues associated with this PR
Describe the problem that is addressed by this PR
What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?
Are there test cases added in this PR? (If not, why?)
Provide a list of related PRs (if any)
Command used to generate this PR:**(Applicable only to SDK release request PRs)
Checklists