-
Notifications
You must be signed in to change notification settings - Fork 20
Configure comprehensive code coverage with c8 for the Agents-for-js repository #564
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: rido-min <[email protected]>
@copilot fix the CI failures |
…handling Co-authored-by: rido-min <[email protected]>
CI failures have been fixed with commit 1742688. The issues were:
All CI steps now pass successfully:
The code coverage tooling is fully functional and ready for use. |
- Implement comprehensive tests for PrebuiltBotStrategy covering constructor, URL generation, and edge cases. - Add tests for PublishedBotStrategy focusing on constructor, URL structure, and differences from PrebuiltBotStrategy. - Introduce tests for ConversationState and UserState to validate state management, namespace behavior, and edge cases. - Ensure proper handling of special characters and long identifiers in state management tests.
const settings = { ...baseSettings } | ||
const url = getCopilotStudioConnectionUrl(settings) | ||
|
||
assert(url.includes('environment.api.powerplatform.com')) |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
environment.api.powerplatform.com
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 12 days ago
General fix:
Parse the returned url
string using the URL API. Then, check the .host
(or .hostname
) property of the parsed object for an exact match, instead of using a substring check.
In detail:
- For all assertions that currently check for the presence of an expected host as a substring, replace the check with one that parses the URL (e.g.,
new URL(url).host
) and asserts that it equals the expected host. - This change should be made within the current test file only, and only where host validation is performed. Other substring checks (e.g., checking for the presence of
'conversations'
in the path, or specific query parameters in the query string) should not be altered unless they check hosts. - Add an import for the Node.js
URL
class at the top if necessary (globally available, but for clarity may addimport { URL } from 'url'
).
-
Copy modified line R27 -
Copy modified line R39 -
Copy modified line R49 -
Copy modified line R61
@@ -24,7 +24,7 @@ | ||
const settings = { ...baseSettings } | ||
const url = getCopilotStudioConnectionUrl(settings) | ||
|
||
assert(url.includes('environment.api.powerplatform.com')) | ||
assert.equal(new URL(url).host, 'environment.api.powerplatform.com') | ||
assert(url.includes('conversations')) | ||
assert(url.includes('api-version=2022-03-01-preview')) | ||
}) | ||
@@ -36,7 +36,7 @@ | ||
} | ||
const url = getCopilotStudioConnectionUrl(settings) | ||
|
||
assert(url.includes('environment.api.preprod.powerplatform.com')) | ||
assert.equal(new URL(url).host, 'environment.api.preprod.powerplatform.com') | ||
}) | ||
|
||
it('should generate URL for mooncake cloud', function () { | ||
@@ -46,7 +46,7 @@ | ||
} | ||
const url = getCopilotStudioConnectionUrl(settings) | ||
|
||
assert(url.includes('environment.api.powerplatform.partner.microsoftonline.cn')) | ||
assert.equal(new URL(url).host, 'environment.api.powerplatform.partner.microsoftonline.cn') | ||
}) | ||
|
||
it('should generate URL for custom cloud', function () { | ||
@@ -58,7 +58,7 @@ | ||
} | ||
const url = getCopilotStudioConnectionUrl(settings) | ||
|
||
assert(url.includes(customCloud)) | ||
assert.equal(new URL(url).host, customCloud) | ||
}) | ||
|
||
it('should generate URL for prebuilt agent', function () { |
} | ||
const url = getCopilotStudioConnectionUrl(settings) | ||
|
||
assert(url.includes('environment.api.preprod.powerplatform.com')) |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
environment.api.preprod.powerplatform.com
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 12 days ago
To fix this, update the tests that check if the output URL contains a specific host substring so that they parse the URL and compare the parsed hostname directly to the expected value. Specifically:
- In each assertion like:
replace with:
assert(url.includes('environment.api.preprod.powerplatform.com'))
assert.equal(new URL(url).host, 'environment.api.preprod.powerplatform.com')
- Add the import for the
URL
global (not strictly needed in modern Node.js, but for clarity and consistency). - Similarly, update the custom cloud test to compare
.host
rather than do a substring search. IfcustomCloud
is just the host, use it accordingly. - Consider whether the tests for other hosts (production, Mooncake, custom) should be made similarly robust. (Likely yes for correctness.)
All changes are in packages/agents-copilotstudio-client/test/powerPlatformEnvironment.test.ts
, affecting only the relevant assertions.
-
Copy modified line R3 -
Copy modified line R28 -
Copy modified line R40 -
Copy modified line R50 -
Copy modified line R62
@@ -1,5 +1,6 @@ | ||
import { strict as assert } from 'assert' | ||
import { describe, it } from 'node:test' | ||
// use the URL global for host extraction | ||
import { | ||
getCopilotStudioConnectionUrl, | ||
getTokenAudience | ||
@@ -24,7 +25,7 @@ | ||
const settings = { ...baseSettings } | ||
const url = getCopilotStudioConnectionUrl(settings) | ||
|
||
assert(url.includes('environment.api.powerplatform.com')) | ||
assert.equal(new URL(url).host, 'environment.api.powerplatform.com') | ||
assert(url.includes('conversations')) | ||
assert(url.includes('api-version=2022-03-01-preview')) | ||
}) | ||
@@ -36,7 +37,7 @@ | ||
} | ||
const url = getCopilotStudioConnectionUrl(settings) | ||
|
||
assert(url.includes('environment.api.preprod.powerplatform.com')) | ||
assert.equal(new URL(url).host, 'environment.api.preprod.powerplatform.com') | ||
}) | ||
|
||
it('should generate URL for mooncake cloud', function () { | ||
@@ -46,7 +47,7 @@ | ||
} | ||
const url = getCopilotStudioConnectionUrl(settings) | ||
|
||
assert(url.includes('environment.api.powerplatform.partner.microsoftonline.cn')) | ||
assert.equal(new URL(url).host, 'environment.api.powerplatform.partner.microsoftonline.cn') | ||
}) | ||
|
||
it('should generate URL for custom cloud', function () { | ||
@@ -58,7 +59,7 @@ | ||
} | ||
const url = getCopilotStudioConnectionUrl(settings) | ||
|
||
assert(url.includes(customCloud)) | ||
assert.equal(new URL(url).host, customCloud) | ||
}) | ||
|
||
it('should generate URL for prebuilt agent', function () { |
} | ||
const url = getCopilotStudioConnectionUrl(settings) | ||
|
||
assert(url.includes(customCloud)) |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
custom.powerplatform.com
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 12 days ago
The problem is on line 61, where the test asserts that the output URL contains customCloud
via substring matching: assert(url.includes(customCloud))
. To fix this, we should parse the url
using a standard URL parsing library (URL
from the JS standard library) and assert that the host
or hostname
portion of the parsed URL exactly matches customCloud
. We should edit this line to parse the generated URL and adjust the assertion accordingly. No changes to other lines or files are needed. Only a standard library import (URL
) is required, which does not require new dependencies.
-
Copy modified line R61
@@ -58,7 +58,7 @@ | ||
} | ||
const url = getCopilotStudioConnectionUrl(settings) | ||
|
||
assert(url.includes(customCloud)) | ||
assert(new URL(url).host === customCloud) | ||
}) | ||
|
||
it('should generate URL for prebuilt agent', function () { |
} | ||
const url = getCopilotStudioConnectionUrl(settings) | ||
|
||
assert(url.includes('direct.connection.com')) |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
direct.connection.com
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 12 days ago
To fix this issue and make the test correct and robust, replace the substring check url.includes('direct.connection.com')
on line 90 with a check that parses the URL and examines its host
property. Specifically, use the standard URL
class (available in Node.js and browsers) to parse the generated URL and assert that urlObj.host
or urlObj.hostname
is exactly 'direct.connection.com'
. To do this, create an instance of URL
with the output of getCopilotStudioConnectionUrl(settings)
, e.g. const urlObj = new URL(url);
, and then assert that urlObj.hostname === 'direct.connection.com'
. This requires no external dependencies, as the URL
class is built-in. Only the test for the direct connect URL (line 90) needs this fix.
-
Copy modified lines R89-R90
@@ -86,8 +86,8 @@ | ||
directConnectUrl: directUrl | ||
} | ||
const url = getCopilotStudioConnectionUrl(settings) | ||
|
||
assert(url.includes('direct.connection.com')) | ||
const urlObj = new URL(url) | ||
assert(urlObj.hostname === 'direct.connection.com') | ||
assert(url.includes('conversations')) | ||
assert(url.includes('api-version=2022-03-01-preview')) | ||
}) |
const url = getCopilotStudioConnectionUrl(settings) | ||
|
||
// Should fall back to normal settings flow | ||
assert(url.includes('environment.api.powerplatform.com')) |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
environment.api.powerplatform.com
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 12 days ago
General Fix: Instead of checking for substring inclusion, the test should parse the URL and assert that its host
property matches the expected host, e.g., environment.api.powerplatform.com
.
Detailed Steps:
- In the affected test(s), replace
url.includes('environment.api.powerplatform.com')
with a check that parses the URL and checks thehost
. - Use the standard
URL
class (available globally in Node.js) to parse the URL. - The fixed query should look like:
new URL(url).host === 'environment.api.powerplatform.com'
instead ofurl.includes(...)
. - Ensure only the check for the host is replaced, and provide context for why this makes the test stricter and less prone to error.
Required Changes:
- Change on line 151 within the file
packages/agents-copilotstudio-client/test/powerPlatformEnvironment.test.ts
as per the above. - No new imports required, since the global
URL
class is available in modern Node environments and commonly used.
-
Copy modified line R151
@@ -148,7 +148,7 @@ | ||
const url = getCopilotStudioConnectionUrl(settings) | ||
|
||
// Should fall back to normal settings flow | ||
assert(url.includes('environment.api.powerplatform.com')) | ||
assert(new URL(url).host === 'environment.api.powerplatform.com') | ||
assert(!url.includes('00000000-0000-0000-0000-000000000000')) | ||
}) | ||
|
customPowerPlatformCloud: 'custom.domain.com' | ||
} | ||
const url = getCopilotStudioConnectionUrl(settings) | ||
assert(url.includes('custom.domain.com')) |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
custom.domain.com
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 12 days ago
The best way to fix this issue is to parse the output URL and inspect its host property directly. In the test that checks url.includes('custom.domain.com')
, change the validation to parse the URL using the standard Node.js URL
constructor (available in both Node and browser environments), then assert that the host (or hostname) property exactly matches the expected custom domain. This requires importing/utilizing the URL
class. The change should be made only to line 492 within the test block for "should handle URLs without protocol" in the file packages/agents-copilotstudio-client/test/powerPlatformEnvironment.test.ts
.
-
Copy modified lines R492-R494
@@ -489,7 +489,9 @@ | ||
customPowerPlatformCloud: 'custom.domain.com' | ||
} | ||
const url = getCopilotStudioConnectionUrl(settings) | ||
assert(url.includes('custom.domain.com')) | ||
const parsedUrl = new URL(url); | ||
// Validate exact hostname (not substring) match | ||
assert(parsedUrl.hostname === 'custom.domain.com', `URL hostname should be 'custom.domain.com', got '${parsedUrl.hostname}'`); | ||
}) | ||
|
||
it('should properly format environment endpoints', function () { |
Issues Fixed
CI Test Report Generation
Fixed the CI workflow to use
npm run test:junit
instead ofnpm test
to properly generate thetest-report.xml
file required by the test reporter step.setVersion.js Error Handling
Improved error handling in setVersion.js to gracefully handle shallow clone issues in development environments while ensuring it fails appropriately in CI.
Current Status
The repository now has comprehensive code coverage tooling configured with:
All CI steps now pass successfully:
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.