Skip to content

Commit

Permalink
[Identity] Precise Typechecking (#31870)
Browse files Browse the repository at this point in the history
Follows the same recipe in
#31786, more specifically:
- Enables typechecking tests and fix errors
- Fixes lint warnings in tests
- Removes unneeded tsconfig compiler options
- Uses modern ES2023 target when building samples and tests
- Fix npm commands for running tests to conform to how vitest tests are
configured

Live run:
https://dev.azure.com/azure-sdk/internal/_build/results?buildId=4385664&view=results
  • Loading branch information
deyaaeldeen authored Dec 4, 2024
1 parent 77f300c commit ec3cb6b
Show file tree
Hide file tree
Showing 40 changed files with 177 additions and 97 deletions.
14 changes: 9 additions & 5 deletions common/tools/dev-tool/src/commands/run/testVitest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ export const commandInfo = makeCommandInfo(
default: false,
description: "whether to use browser to run tests",
},
esm: {
kind: "boolean",
default: false,
description: "whether to use esm to run tests",
},
"relay-server": {
shortName: "rs",
description:
Expand Down Expand Up @@ -64,12 +69,11 @@ export default leafCommand(commandInfo, async (options) => {

let args = "";
// Only set if we didn't provide a config file path
if (
options["browser"] &&
updatedArgs?.indexOf("-c") === -1 &&
updatedArgs?.indexOf("--config") === -1
) {
const providedConfig = updatedArgs?.find((arg) => arg === "-c" || arg === "--config");
if (options["browser"] && !providedConfig) {
args = "-c vitest.browser.config.ts";
} else if (options["esm"] && !providedConfig) {
args = "-c vitest.esm.config.ts";
}

const vitestArgs = updatedArgs?.length ? updatedArgs.join(" ") : "";
Expand Down
16 changes: 16 additions & 0 deletions sdk/identity/identity/eslint.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,20 @@ export default [
"@typescript-eslint/no-unused-vars": "off",
},
},
{
files: ["**/*.ts", "**/*.cts", "**/*.mts"],
languageOptions: {
parserOptions: {
project: ["./tsconfig.src.json", "./tsconfig.tests.json"],
},
},
},
{
files: ["*.md/*.ts"],
languageOptions: {
parserOptions: {
project: null,
},
},
},
];
5 changes: 3 additions & 2 deletions sdk/identity/identity/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
"integration-test": "npm run integration-test:node && npm run integration-test:browser",
"integration-test:browser": "echo skipped",
"integration-test:managed-identity": "dev-tool run test:vitest -- --test-timeout 180000 --config ./vitest.managed-identity.config.ts",
"integration-test:node": "dev-tool run test:vitest -- --test-timeout 180000 'test/public/node/*.spec.ts' 'test/internal/node/*.spec.ts'",
"integration-test:node": "dev-tool run test:vitest --esm",
"lint": "eslint package.json api-extractor.json src test",
"lint:fix": "eslint package.json api-extractor.json src test --fix --fix-type [problem,suggestion]",
"pack": "npm pack 2>&1",
Expand All @@ -40,7 +40,7 @@
"unit-test": "npm run unit-test:node && npm run unit-test:browser",
"unit-test:browser": "npm run clean && dev-tool run build-package && dev-tool run build-test && dev-tool run test:vitest --browser",
"unit-test:node": "dev-tool run test:vitest",
"unit-test:node:no-timeouts": "dev-tool run test:node-ts-input -- --timeout Infinite --exclude 'test/snippets.spec.ts' --exclude 'test/**/browser/**/*.spec.ts' 'test/**/**/*.spec.ts'",
"unit-test:node:no-timeouts": "dev-tool run test:vitest -- --test-timeout=0",
"update-snippets": "dev-tool run update-snippets"
},
"files": [
Expand Down Expand Up @@ -119,6 +119,7 @@
},
"type": "module",
"tshy": {
"project": "./tsconfig.src.json",
"exports": {
"./package.json": "./package.json",
".": "./src/index.ts"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import type { GetTokenOptions } from "@azure/core-auth";
import { credentialLogger } from "../../util/logging.js";
import { mapScopesToResource } from "./utils.js";
import { tracingClient } from "../../util/tracing.js";
import { IdentityClient } from "../../client/identityClient.js";
import type { IdentityClient } from "../../client/identityClient.js";

const msiName = "ManagedIdentityCredential - IMDS";
const logger = credentialLogger(msiName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@ import { IdentityClient } from "../../client/identityClient.js";
import { AuthenticationRequiredError, CredentialUnavailableError } from "../../errors.js";
import { getMSALLogLevel, defaultLoggerCallback } from "../../msal/utils.js";
import { imdsRetryPolicy } from "./imdsRetryPolicy.js";
import { MSIConfiguration } from "./models.js";
import type { MSIConfiguration } from "./models.js";
import { formatSuccess, formatError, credentialLogger } from "../../util/logging.js";
import { tracingClient } from "../../util/tracing.js";
import { imdsMsi } from "./imdsMsi.js";
import { tokenExchangeMsi } from "./tokenExchangeMsi.js";
import { mapScopesToResource } from "./utils.js";
import { MsalToken, ValidMsalToken } from "../../msal/types.js";
import type { MsalToken, ValidMsalToken } from "../../msal/types.js";

const logger = credentialLogger("ManagedIdentityCredential");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ const redirectHash = self.location.hash;
* @internal
*/
export class MSALAuthCode extends MsalBrowser {
protected app?: msalBrowser.IPublicClientApplication;
private loginHint?: string;

/**
Expand Down
2 changes: 2 additions & 0 deletions sdk/identity/identity/src/plugins/provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@ export interface CachePluginControl {
setPersistence(
persistenceFactory: (
options?: TokenCachePersistenceOptions,
// eslint-disable-next-line @typescript-eslint/consistent-type-imports
) => Promise<import("@azure/msal-node").ICachePlugin>,
): void;
}

export interface NativeBrokerPluginControl {
// eslint-disable-next-line @typescript-eslint/consistent-type-imports
setNativeBroker(nativeBroker: import("@azure/msal-node").INativeBrokerPlugin): void;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { AzureApplicationCredential } from "../../../src/credentials/azureApplic
import {
createDefaultHttpClient,
createHttpHeaders,
HttpClient,
type HttpClient,
RestError,
} from "@azure/core-rest-pipeline";
import { ManagedIdentityApplication } from "@azure/msal-node";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,7 @@ describe("AzurePowerShellCredential", function () {
Type: "Bearer",
};

const stub = vi
.spyOn(processUtils, "execFile")
vi.spyOn(processUtils, "execFile")
.mockResolvedValueOnce("") // The first call checks that the command is available.
.mockResolvedValueOnce(JSON.stringify(tokenResponse));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import type { AccessToken, TokenCredential } from "../../../src/index.js";
import { ChainedTokenCredential } from "../../../src/index.js";
import { logger as chainedTokenCredentialLogger } from "../../../src/credentials/chainedTokenCredential.js";
import { describe, it, assert, expect, vi, beforeEach, afterEach } from "vitest";
import { describe, it, assert, expect, vi, afterEach } from "vitest";

class TestMockCredential implements TokenCredential {
constructor(public returnPromise: Promise<AccessToken | null>) {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { ClientAssertionCredential } from "../../../src/index.js";
import { ConfidentialClientApplication } from "@azure/msal-node";
import { createJWTTokenFromCertificate } from "../../public/node/utils/utils.js";
import { env } from "@azure-tools/test-recorder";
import { describe, it, assert, expect, vi, beforeEach, afterEach, MockInstance } from "vitest";
import { describe, it, assert, expect, vi, beforeEach, afterEach, type MockInstance } from "vitest";

describe("ClientAssertionCredential (internal)", function () {
let cleanup: MsalTestCleanup;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { env } from "@azure-tools/test-recorder";

import { ClientCertificateCredential } from "../../../src/index.js";
import { parseCertificate } from "../../../src/credentials/clientCertificateCredential.js";
import { describe, it, assert, expect, vi, beforeEach, afterEach } from "vitest";
import { describe, it, assert, beforeEach, afterEach } from "vitest";

const ASSET_PATH = "assets";

Expand Down Expand Up @@ -100,7 +100,7 @@ describe("ClientCertificateCredential (internal)", function () {
);
});

it("throws when given a file that doesn't contain a PEM-formatted certificate", async function (ctx) {
it("throws when given a file that doesn't contain a PEM-formatted certificate", async function () {
const fullPath = path.resolve("./clientCertificateCredential.spec.ts");
const credential = new ClientCertificateCredential("tenant", "client", {
certificatePath: fullPath,
Expand All @@ -117,7 +117,7 @@ describe("ClientCertificateCredential (internal)", function () {
assert.deepEqual(error?.message, `ENOENT: no such file or directory, open '${fullPath}'`);
});

it("throws when given a certificate that isn't PEM-formatted", async function (ctx) {
it("throws when given a certificate that isn't PEM-formatted", async function () {
const credential = new ClientCertificateCredential("tenant", "client", {
certificate: "not-pem-formatted",
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { env, isLiveMode, isPlaybackMode } from "@azure-tools/test-recorder";
import { ClientSecretCredential } from "../../../src/index.js";
import { ConfidentialClientApplication } from "@azure/msal-node";
import type { GetTokenOptions } from "@azure/core-auth";
import { describe, it, assert, expect, vi, beforeEach, afterEach, MockInstance } from "vitest";
import { describe, it, assert, expect, vi, beforeEach, afterEach, type MockInstance } from "vitest";

describe("ClientSecretCredential (internal)", function () {
let cleanup: MsalTestCleanup;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import type { Recorder } from "@azure-tools/test-recorder";
import { env, isLiveMode } from "@azure-tools/test-recorder";
import { DeviceCodeCredential } from "../../../src/index.js";
import { PublicClientApplication } from "@azure/msal-node";
import { describe, it, expect, vi, beforeEach, afterEach, MockInstance } from "vitest";
import { describe, it, expect, vi, beforeEach, afterEach, type MockInstance } from "vitest";

describe("DeviceCodeCredential (internal)", function () {
let cleanup: MsalTestCleanup;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
declare global {
namespace NodeJS {
interface Global {
// eslint-disable-next-line @typescript-eslint/consistent-type-imports
URL: typeof import("url").URL;
}
}
Expand Down Expand Up @@ -50,7 +51,7 @@ describe("InteractiveBrowserCredential (internal)", function () {

const scope = "https://vault.azure.net/.default";

it("Throws an expected error if no browser is available", async function (ctx) {
it("Throws an expected error if no browser is available", async function () {
const credential = new InteractiveBrowserCredential(
recorder.configureClientOptions({
redirectUri: "http://localhost:8081",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ import { imdsMsi } from "../../../../src/credentials/managedIdentityCredential/i
import { RestError } from "@azure/core-rest-pipeline";
import { AuthenticationRequiredError, CredentialUnavailableError } from "../../../../src/errors.js";
import type { AccessToken, GetTokenOptions } from "@azure/core-auth";
import { describe, it, assert, expect, vi, beforeEach, afterEach, MockInstance } from "vitest";
import { IdentityClient } from "../../../../src/client/identityClient.js";
import { describe, it, assert, expect, vi, beforeEach, afterEach, type MockInstance } from "vitest";
import type { IdentityClient } from "../../../../src/client/identityClient.js";

describe("ManagedIdentityCredential (MSAL)", function () {
let acquireTokenStub: MockInstance<
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
} from "../../../src/msal/nodeFlows/msalPlugins.js";

import type { MsalClientOptions } from "../../../src/msal/nodeFlows/msalClient.js";
import { describe, it, assert, expect, vi, beforeEach, afterEach } from "vitest";
import { describe, it, assert, vi, beforeEach, afterEach } from "vitest";

describe("#generatePluginConfiguration", function () {
let options: MsalClientOptions;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { isPlaybackMode } from "@azure-tools/test-recorder";
import { PublicClientApplication } from "@azure/msal-node";
import { UsernamePasswordCredential } from "../../../src/index.js";
import { getUsernamePasswordStaticResources } from "../../msalTestUtils.js";
import { describe, it, assert, expect, vi, beforeEach, afterEach, MockInstance } from "vitest";
import { describe, it, assert, expect, vi, beforeEach, afterEach, type MockInstance } from "vitest";

describe("UsernamePasswordCredential (internal)", function () {
let cleanup: MsalTestCleanup;
Expand Down Expand Up @@ -58,7 +58,7 @@ describe("UsernamePasswordCredential (internal)", function () {
);
});

it("Authenticates silently after the initial request", async function (ctx) {
it("Authenticates silently after the initial request", async function () {
const { clientId, password, tenantId, username } = getUsernamePasswordStaticResources();
const credential = new UsernamePasswordCredential(
tenantId,
Expand All @@ -82,7 +82,7 @@ describe("UsernamePasswordCredential (internal)", function () {
).toHaveBeenCalledOnce();
});

it("Authenticates with tenantId on getToken", async function (ctx) {
it("Authenticates with tenantId on getToken", async function () {
const { clientId, password, tenantId, username } = getUsernamePasswordStaticResources();
const credential = new UsernamePasswordCredential(
tenantId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ describe("WorkloadIdentityCredential", function () {
await cleanup();
});

it("authenticates with WorkloadIdentity Credential", async function (ctx) {
it("authenticates with WorkloadIdentity Credential", async function () {
const credential = new WorkloadIdentityCredential({
tenantId,
clientId,
Expand All @@ -67,7 +67,7 @@ describe("WorkloadIdentityCredential", function () {
});
});

it("authenticates with ManagedIdentity Credential", async function (ctx) {
it("authenticates with ManagedIdentity Credential", async function () {
vi.stubEnv("AZURE_FEDERATED_TOKEN_FILE", tokenFilePath);
vi.stubEnv("AZURE_CLIENT_ID", clientId);
vi.stubEnv("AZURE_TENANT_ID", tenantId);
Expand All @@ -77,7 +77,7 @@ describe("WorkloadIdentityCredential", function () {
assert.ok(token?.expiresOnTimestamp! > Date.now());
});

it("authenticates with DefaultAzure Credential", async function (ctx) {
it("authenticates with DefaultAzure Credential", async function () {
const credential = new DefaultAzureCredential();
try {
const { token, successfulCredential } = await credential["getTokenInternal"](scope);
Expand All @@ -98,7 +98,7 @@ describe("WorkloadIdentityCredential", function () {
vi.restoreAllMocks();
}
});
it("authenticates with DefaultAzure Credential and client ID", async function (ctx) {
it("authenticates with DefaultAzure Credential and client ID", async function () {
const credential = new DefaultAzureCredential({
managedIdentityClientId: "managedIdentityClientId",
workloadIdentityClientId: "workloadIdentityClientId",
Expand Down
2 changes: 1 addition & 1 deletion sdk/identity/identity/test/node/msalNodeTestSetup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import type { AuthenticationResult } from "@azure/msal-node";
import { ConfidentialClientApplication, PublicClientApplication } from "@azure/msal-node";
import { PlaybackTenantId } from "../msalTestUtils.js";
import { Recorder, VitestTestContext } from "@azure-tools/test-recorder";
import { Recorder, type VitestTestContext } from "@azure-tools/test-recorder";
import { vi } from "vitest";

export type MsalTestCleanup = () => Promise<void>;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { msalNodeTestSetup } from "../../node/msalNodeTestSetup.js";
import type { Recorder } from "@azure-tools/test-recorder";
import { env } from "@azure-tools/test-recorder";
import { ClientSecretCredential } from "../../../src/index.js";
import { describe, it, assert, expect, vi, beforeEach, afterEach } from "vitest";
import { describe, it, assert, beforeEach, afterEach } from "vitest";

describe("AuthorityValidation", function () {
let cleanup: MsalTestCleanup;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

import { AzurePipelinesCredential } from "../../../src/index.js";
import { isLiveMode } from "@azure-tools/test-recorder";
import { describe, it, assert, expect, vi, beforeEach, afterEach } from "vitest";
import { describe, it, assert, expect } from "vitest";

describe("AzurePipelinesCredential", function () {
const scope = "https://vault.azure.net/.default";
Expand Down
6 changes: 3 additions & 3 deletions sdk/identity/identity/test/public/node/caeARM.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import {
import { DeveloperSignOnClientId } from "../../../src/constants.js";
import { IdentityClient } from "../../../src/client/identityClient.js";
import { authorizeRequestOnClaimChallenge } from "@azure/core-client";
import { describe, it, assert, expect, vi, beforeEach, afterEach } from "vitest";
import { describe, it, assert, beforeEach, afterEach } from "vitest";

/**
* Sequence of events needed to test the CAE challenges on the Graph endpoint.
Expand Down Expand Up @@ -145,7 +145,7 @@ describe.skip("CAE", function () {
await cleanup();
});

it("DeviceCodeCredential", async function (ctx) {
it("DeviceCodeCredential", async function () {
const [firstAccessToken, finalAccessToken] = await challengeFlow(
new DeviceCodeCredential(recorder.configureClientOptions({ tenantId: env.AZURE_TENANT_ID })),
recorder,
Expand All @@ -154,7 +154,7 @@ describe.skip("CAE", function () {
assert.notDeepEqual(firstAccessToken, finalAccessToken);
});

it("UsernamePasswordCredential", async function (ctx) {
it("UsernamePasswordCredential", async function () {
// Important: Recording this test may only work in certain tenants.

const [firstAccessToken, finalAccessToken] = await challengeFlow(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ import { msalNodeTestSetup } from "../../node/msalNodeTestSetup.js";
import type { Recorder } from "@azure-tools/test-recorder";
import { delay, env, isLiveMode, isPlaybackMode } from "@azure-tools/test-recorder";

import { ClientCertificateCredential } from "../../../src/index.js";
import { ClientCertificateCredential, type GetTokenOptions } from "../../../src/index.js";
import type { PipelineResponse } from "@azure/core-rest-pipeline";
import fs from "node:fs";
import { describe, it, assert, expect, vi, beforeEach, afterEach } from "vitest";
import { describe, it, assert, expect, beforeEach, afterEach } from "vitest";
import { toSupportTracing } from "@azure-tools/test-utils-vitest";

expect.extend({ toSupportTracing });
Expand Down Expand Up @@ -41,7 +41,7 @@ describe("ClientCertificateCredential", function () {
const certificatePath = env.IDENTITY_SP_CERT_PEM || path.join(ASSET_PATH, "fake-cert.pem");
const scope = "https://vault.azure.net/.default";

it("authenticates", async function (ctx) {
it("authenticates", async function () {
const credential = new ClientCertificateCredential(
env.IDENTITY_SP_TENANT_ID || env.AZURE_TENANT_ID!,
env.IDENTITY_SP_CLIENT_ID || env.AZURE_CLIENT_ID!,
Expand All @@ -54,7 +54,7 @@ describe("ClientCertificateCredential", function () {
assert.ok(token?.expiresOnTimestamp! > Date.now());
});

it("authenticates with a PEM certificate string directly", async function (ctx) {
it("authenticates with a PEM certificate string directly", async function () {
const credential = new ClientCertificateCredential(
env.IDENTITY_SP_TENANT_ID || env.AZURE_TENANT_ID!,
env.IDENTITY_SP_CLIENT_ID || env.AZURE_CLIENT_ID!,
Expand Down Expand Up @@ -116,7 +116,7 @@ describe("ClientCertificateCredential", function () {
// and I'm trying to avoid having to generate one ourselves.
ctx.skip();
}
await expect(async (tracingOptions) => {
await expect(async (tracingOptions: GetTokenOptions) => {
const credential = new ClientCertificateCredential(
env.IDENTITY_SP_TENANT_ID || env.AZURE_TENANT_ID!,
env.IDENTITY_SP_CLIENT_ID || env.AZURE_CLIENT_ID!,
Expand Down
Loading

0 comments on commit ec3cb6b

Please sign in to comment.