[identity] Improve the testing framework #35349

deyaaeldeen · 2025-07-25T04:29:46Z

This PR improves type-checking and efficiency of our tests and tightens the feedback loop for platform-specific issues. Some of the issues addressed in this PR:

browser tests build the source code twice unnecessarily, once using the build-package dev-tool command and another using the build-test command.
browser tests aren't being type-checked precisely, Node.js APIs calls aren't being forbidden by the typechecker.
Node.js tests aren't being type-checked precisely either, DOM calls aren't being forbidden by the typechecker.

Key Changes:

Splits test configurations into separate Node.js (tsconfig.test.node.json) and browser (tsconfig.browser.config.json) configurations to enable precise type-checking.
Implements consistent TypeScript path mappings using @azure/identity and internal/* aliases across all test files. This aliasing enables testing built code or source code in a modular way.
Updates Vitest configurations with dynamic path resolution for both Node.js and browser environments so that Node.js tests test against the source code locally and against the built code in test pipelines. Similarly, the browser tests test against the browser build. This reduces compilation overhead as there is no longer a need to rebuild the source code when building the browser tests.

Copilot

Pull Request Overview

This PR improves the testing framework for the Azure Identity package by implementing separate TypeScript configurations for Node.js and browser environments, enabling precise type-checking and reducing build overhead. The changes eliminate unnecessary double compilation in browser tests and provide better separation of concerns between different runtime environments.

Key changes:

Splits test configurations into separate Node.js and browser TypeScript configs with environment-specific type checking
Implements consistent path aliasing using @azure/identity and internal/* patterns across all test files
Updates Vitest configurations with dynamic path resolution to test against source code locally and built code in CI pipelines

Reviewed Changes

Copilot reviewed 60 out of 60 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
vitest.esm.config.ts	Removed ESM-specific Vitest configuration file
vitest.config.ts	Added dynamic path resolution and alias configuration for testing
vitest.browser.config.ts	Updated browser test configuration with new alias pattern
tsconfig.test.node.json	New Node.js-specific test configuration with precise type checking
tsconfig.test.json	Converted to project references structure
tsconfig.browser.config.json	New browser-specific test configuration
test/*/.spec.ts	Updated import statements to use new aliasing pattern
package.json	Simplified test scripts by removing ESM-specific commands
build-test.ts	Enhanced build tool with better TypeScript config handling

sdk/identity/identity/src/errors.ts

sdk/identity/identity/eslint.config.mjs

sdk/identity/identity/vitest.config.ts

sdk/identity/identity/tsconfig.browser.config.json

xirzec

I love being able to avoid the extra build step for browser! Left some minor comments.

xirzec · 2025-07-29T19:05:04Z

sdk/identity/identity/vitest.config.ts

+          replacement: resolve(__dirname, `./${dist}/${indexFile}`),
+        },
+        {
+          find: /^internal\/(.*)$/,


could we rename this to something more obviously a pseudo-module like $internal or test:internal

addressed in 4e9bd19

xirzec · 2025-07-29T19:05:45Z

sdk/identity/identity/vitest.config.ts

 import { defineConfig, mergeConfig } from "vitest/config";
 import viteConfig from "../../../vitest.shared.config.ts";

+const isCI = process.env["SYSTEM_TEAMPROJECTID"] !== undefined;


would love to have a helper function/module for this we could share between vitest configs, maybe in the shared config itself?

addressed in 4e9bd19

xirzec · 2025-07-29T22:16:51Z

sdk/identity/identity/src/errors.ts

@@ -76,7 +76,7 @@ export const CredentialUnavailableErrorName = "CredentialUnavailableError";
 */
 export class CredentialUnavailableError extends Error {
  constructor(message?: string, options?: { cause?: unknown }) {
-    // @ts-expect-error - TypeScript does not recognize this until we use ES2022 as the target; however, all our major runtimes do support the `cause` property
+    // @ts-ignore - TypeScript does not recognize this until we use ES2022 as the target; however, all our major runtimes do support the `cause` property


maybe we don't need this anymore now that https://github.com/Azure/azure-sdk-for-js/pull/35387/files is merged?

Yup, it is no longer needed, addressed in 4e9bd19

common/tools/dev-tool/src/commands/run/build-test.ts

jeremymeng · 2025-07-29T22:57:46Z

common/tools/dev-tool/src/commands/run/build-test.ts

-    const browserConfigStat = statSync(browserConfig);
-    if (!browserConfigStat.isFile()) {
-      log.error(`The file ${browserConfig} does not exist.`);
+    if (!(await validateConfigFile(browserConfig))) {


nit: maybe combine the two ifs?

we still need to do other logic in the browser tests case.

jeremymeng · 2025-07-30T00:41:11Z

sdk/identity/identity/vitest.browser.config.ts

-      exclude: [
-        "dist-test/browser/test/snippets.spec.js",
-        "dist-test/browser/test/integration/**/*.spec.js"
+      alias: [


One thing I am not sure whether it would work well with these aliases is the "Add imports" IDE feature that allows you to Ctrl+<DOT> or mouse click to add missing imports. Would the IDE know to add/update imports to these aliased paths? If not would tests be broken when imports to, e.g., "../../*.js" are added by IDE?

Yeah that can be annoying. In my experience, more often than not, the IDE doesn't import from the right places anyway and you need to go and edit the import. For example, when importing a definition exported by the library, the IDE may decide to import it from the defining module instead of src/index.

github-actions bot added Azure.Identity dev-tool Issues related to the Azure SDK for JS dev-tool labels Jul 25, 2025

github-project-automation bot added this to Azure Identity SDK Improvements Jul 25, 2025

github-project-automation bot moved this to Untriaged in Azure Identity SDK Improvements Jul 25, 2025

deyaaeldeen force-pushed the identity/modular-tests branch 4 times, most recently from ad8dbac to 8f99b70 Compare July 25, 2025 23:59

[identity] src/build testing

ced0a2a

deyaaeldeen force-pushed the identity/modular-tests branch from 8f99b70 to ced0a2a Compare July 25, 2025 23:59

deyaaeldeen changed the title ~~[identity] src/build testing~~ [identity] Improve the testing framework Jul 26, 2025

deyaaeldeen marked this pull request as ready for review July 28, 2025 16:57

Copilot AI review requested due to automatic review settings July 28, 2025 16:57

deyaaeldeen requested review from mikeharder, weshaggard, benbp, jeremymeng, timovv, maorleger, KarishmaGhiya, minhanh-phan and a team as code owners July 28, 2025 16:57

deyaaeldeen requested review from pvaneck, scottaddie, chlowell and anannya03 July 28, 2025 16:57

Copilot AI reviewed Jul 28, 2025

View reviewed changes

sdk/identity/identity/src/errors.ts Outdated Show resolved Hide resolved

sdk/identity/identity/src/errors.ts Outdated Show resolved Hide resolved

sdk/identity/identity/src/errors.ts Outdated Show resolved Hide resolved

sdk/identity/identity/eslint.config.mjs Outdated Show resolved Hide resolved

deyaaeldeen commented Jul 28, 2025

View reviewed changes

sdk/identity/identity/vitest.config.ts Outdated Show resolved Hide resolved

deyaaeldeen commented Jul 28, 2025

View reviewed changes

sdk/identity/identity/tsconfig.browser.config.json Show resolved Hide resolved

xirzec approved these changes Jul 29, 2025

View reviewed changes

xirzec reviewed Jul 29, 2025

View reviewed changes

jeremymeng reviewed Jul 30, 2025

View reviewed changes

deyaaeldeen added 4 commits July 29, 2025 18:45

Merge remote-tracking branch 'origin/main' into identity/modular-tests

b7614cd

address feedback

4e9bd19

revert change in eslint config

0f366e9

edit

0b80065

[identity] Improve the testing framework #35349

Are you sure you want to change the base?

[identity] Improve the testing framework #35349

Uh oh!

Conversation

deyaaeldeen commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xirzec left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

deyaaeldeen commented Jul 25, 2025 •

edited

Loading