Improve test coverage to ~100% for @azure/core-client#38176
Merged
deyaaeldeen merged 38 commits intomainfrom Apr 22, 2026
Merged
Improve test coverage to ~100% for @azure/core-client#38176deyaaeldeen merged 38 commits intomainfrom
deyaaeldeen merged 38 commits intomainfrom
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Buffer is not available in browser environments. Guard the Buffer-specific test with it.runIf and use a literal base64 string for Uint8Array assertion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Split base64.spec.ts: Buffer test moved to test/internal/node/base64.spec.ts, cross-platform Uint8Array test stays in test/internal/base64.spec.ts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- serviceClient.spec.ts: Replace weak assert.isDefined with specific assertions verifying Invalid Date instance for malformed DateTime string - authorizeRequestOnTenantChallenge.spec.ts: Fix const->let for calledOnce variable so both if/else branches are exercised in mock Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… test Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- pipeline: isTrue(length > 0) → isAbove(length, 0) - serializer: equal(x, false) → strictEqual(x, false) - deserializationPolicy: ok(result) → equal(result._response.status, ...) - deserializationPolicy: add result assertions to no-op tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- serializer: add assert.fail to 3 try/catch blocks, assert() → assert.exists() - deserializationPolicy: assert(e) → assert.exists(e) - serializationPolicy: assert.ok(capturedRequest) → assert.exists(capturedRequest) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Fix TS18046: cast result to any for _response access → use isDefined - Fix formatting in serviceClient.spec.ts Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nt tests Use vi.fn() to capture sendRequest calls instead of the error-prone 'let request; ... request = req' pattern. This eliminates all non-null - Tests that assert on captured request now use sendRequest.mock.calls[0][0] - Tests where request was captured but never asserted have the capture removed Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…viceClient tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace try/catch + assert.fail() with rejects.toThrow/toMatchObject and assert.throws() for cleaner, more idiomatic test code. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- deepEqual→strictEqual for primitive comparisons - Remove requestFailed boolean flag pattern - assert.equal(x, undefined)→assert.isUndefined - calledOnce flag→vi.fn() with mockImplementationOnce Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Replace try/catch + assert.fail with expect().rejects.toThrow/toMatchObject - Replace assert.deepEqual for string comparisons with assert.strictEqual Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR significantly expands the automated test suite for @azure/core-client to drive coverage closer to 100%, mainly by adding targeted unit tests for previously untested branches and improving existing assertions/mocking patterns.
Changes:
- Added multiple new internal/public test files to cover core-client utilities, pipeline creation, URL helpers, Base64 helpers, and operation/interface helpers.
- Expanded existing tests for serialization/deserialization policies and the serializer to cover more edge cases (XML/JSON parsing paths, polymorphism, constraints, stream bodies, etc.).
- Refactored some tests to use more robust Vitest patterns (
vi.fn().mockImplementationOnce,rejects.toThrow,strictEqual, etc.).
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| sdk/core/core-client/test/utils/serviceClient.ts | Simplifies request capture in shared ServiceClient test helper. |
| sdk/core/core-client/test/public/serializer.spec.ts | Large expansion of serializer coverage (base64url, date/stream/enum/sequence/dictionary/composite/xml branches). |
| sdk/core/core-client/test/public/authorizeRequestOnTenantChallenge.spec.ts | Improves mocking of next with vi.fn().mockImplementationOnce for multi-call flows. |
| sdk/core/core-client/test/internal/utils.spec.ts | Adds tests for flattenResponse paging + stream response shaping. |
| sdk/core/core-client/test/internal/urlHelpers.spec.ts | Adds tests for additional appendQueryParams and appendPath edge cases. |
| sdk/core/core-client/test/internal/serviceClient.spec.ts | Refactors several tests toward expect(...).toThrow / rejects patterns and tighter assertions. |
| sdk/core/core-client/test/internal/serializationPolicy.spec.ts | Adds broader serialization-policy coverage (form data, XML handling, stream handling, custom headers, error paths). |
| sdk/core/core-client/test/internal/pipeline.spec.ts | Adds tests for createClientPipeline behavior with/without credentials and default options. |
| sdk/core/core-client/test/internal/operationHelpers.spec.ts | Adds tests for composite parameter paths and originalRequest symbol behavior. |
| sdk/core/core-client/test/internal/interfaceHelpers.spec.ts | Adds coverage for getPathStringFromParameter fallback behavior. |
| sdk/core/core-client/test/internal/deserializationPolicy.spec.ts | Adds many new deserialization-policy tests for parsing errors, XML branches, header mappers, and response selection. |
| sdk/core/core-client/test/internal/base64.spec.ts | Adds base64 helper test for Uint8Array. |
| sdk/core/core-client/test/internal/node/base64.spec.ts | Adds base64 helper test for Node Buffer input (node-only). |
| sdk/core/core-client/test/internal/authorizeRequestOnClaimChallenge.spec.ts | Expands CAE/WWW-Authenticate parsing edge cases and reduces request creation duplication. |
| sdk/core/core-client-rest/test/internal/clientHelpers.spec.ts | Adds regression test to avoid mis-detecting non-string key as KeyCredential. |
Comments suppressed due to low confidence (2)
sdk/core/core-client/test/internal/serviceClient.spec.ts:101
requestis declared and only assigned within thesendRequestcallback, but never read. WithnoUnusedLocals: truethis will fail TypeScript compilation. Remove the variable (and any related capture) from this test since the constructor is expected to throw before any request is sent.
expect(() => {
let request: OperationRequest;
new ServiceClient({
httpClient: {
sendRequest: (req) => {
request = req;
return Promise.resolve({ request, status: 200, headers: createHttpHeaders() });
},
sdk/core/core-client/test/internal/serializationPolicy.spec.ts:255
- There are two consecutive test cases with the identical title "should serialize a JSON Stream request body". This makes it hard to identify which case failed when a test breaks. Rename one of them to reflect the specific behavior under test (e.g., the xmlNamespace-related case).
it("should serialize a JSON Stream request body", () => {
const httpRequest = defaultRequest();
serializeRequestBody(
httpRequest,
{
bodyArg: "body value",
},
{
httpMethod: "POST",
requestBody: {
parameterPath: "bodyArg",
mapper: {
required: true,
serializedName: "bodyArg",
type: {
name: MapperTypeNames.Stream,
},
},
},
responses: { 200: {} },
serializer: createSerializer(),
},
stringifyXML,
);
assert.strictEqual(httpRequest.body, "body value");
});
it("should serialize a JSON Stream request body", () => {
const httpRequest = defaultRequest();
serializeRequestBody(
httpRequest,
{
bodyArg: "body value",
},
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- authorizeRequestOnClaimChallenge: 'empty' -> 'missing' WWW-Authenticate header - deserializationPolicy: fix 4 misleading titles (operationResponseGetter, HEAD streaming, stream status codes, empty operationSpec) - urlHelpers: fix 3 misleading titles (dedup, unshift, trailing slash) - serializer: 'parse NaN' -> 'return non-numeric string as-is' Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Assert XML body content instead of just exists/isString - Add error message patterns to bare toThrow() calls Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jeremymeng
reviewed
Apr 22, 2026
- Replace assert.ok(pipeline) with assert.isDefined(pipeline) - Fix shouldDeserialize callback type: any → PipelineResponse - Rename 'error in error deserialization' → 'missing className in error body mapper' - Rename 'pageable array response' → 'array response' (no pageable in test) - Move unparseable challenge test from parseCAEChallenge to parent describe - Strengthen urlHelpers assertions: add negative assertions, verify exact values - Strengthen serializer date/time assertions: use strictEqual over isString/include - Add positive validateConstraints test case alongside all-negative throwing tests - Prove operationHelpers transformation: add extraProp not in result - Inline defaultRequest() helper in serializationPolicy per review feedback - Add clarifying comment on xmlElementName test explaining faked parseXML - Strengthen default response test assertion Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace assert.include partial matches with assert.strictEqual for full body value verification in XML serialization tests (Sequence, Namespace, String). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jeremymeng
approved these changes
Apr 22, 2026
deyaaeldeen
added a commit
that referenced
this pull request
Apr 23, 2026
Fix two issues in the Array element metadata error messages in `serializer.ts`: - Add missing opening quote: `element"` → `"element"` - Fix grammar: "it must of type" → "it must be of type" Both occurrences (serialize and deserialize paths) are fixed. Test regexes updated to match. Fixes feedback from #38176. Co-authored-by: Deyaaeldeen Almahallawi <deyaa@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
New test files
Expanded existing tests
Test quality improvements