[identity] Use esm4mocha, re-enable IBC test, remove unnecessary stub #28580

maorleger · 2024-02-14T21:52:58Z

Packages impacted by this PR

@azure/identity

Issues associated with this PR

Describe the problem that is addressed by this PR

This PR addresses 3 things, all thanks to @jeremymeng's work on esm4mocha:

Re-enables IBC test that was skipped due to [Identity] Re-enable interactiveBrowserCredential test #28373
Removes sinon stubbing of uuid, replacing it with recorder replacer
Fixes min/max tests for identity

Unfortunately we are not out of the woods regarding msal-node and esm. We would need to get everyone migrated over
to this loader before we can upgrade msal-node repo-wide :(

Provide a list of related PRs (if any)

#28556

Command used to generate this PR: (Applicable only to SDK release request PRs)

Checklists

Added impacted package name to the issue description.
Does this PR need any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here.)
Added a changelog (if necessary).

common/tools/dev-tool/src/commands/run/testNodeJSInput.ts

maorleger · 2024-02-14T21:55:53Z

common/tools/esm4mocha.mjs

@@ -32,7 +33,8 @@ export async function load(url, context, defaultLoad) {
    };
  }
  const { source } = await defaultLoad(url, context, defaultLoad);
-  return { format: context.format, source, shortCircuit: true };
+  const format = context.format ?? "builtin";


node:fs/promises wasn't importing correctly for me. I was thinking it's fine to default to builtin? Alternatively I can also do something like:

const format = context.format if (format === undefined && url.startsWith("node:")) format = "builtin"

Or if anyone has a better suggestion, I am not too familiar with loader logic

interesting! My changes are barely minimum to get many packages test running. I am sure there are cases that aren't handled properly. If buildin works we will use it!

maorleger · 2024-02-14T21:56:42Z

sdk/identity/identity/package.json

@@ -55,7 +55,7 @@
    "format": "dev-tool run vendored prettier --write --config ../../../.prettierrc.json --ignore-path ../../../.prettierignore \"src/**/*.ts\" \"test/**/*.ts\" \"samples-dev/**/*.ts\" \"*.{js,json}\"",
    "check-format": "dev-tool run vendored prettier --list-different --config ../../../.prettierrc.json --ignore-path ../../../.prettierignore \"src/**/*.ts\" \"test/**/*.ts\" \"samples-dev/**/*.ts\" \"*.{js,json}\"",
    "integration-test:browser": "echo skipped",
-    "integration-test:node": "dev-tool run test:node-js-input -- --timeout 180000 'dist-esm/test/public/node/*.spec.js' 'dist-esm/test/internal/node/*.spec.js'",
+    "integration-test:node": "dev-tool run test:node-js-input --no-esm=true -- --loader=../../../common/tools/esm4mocha.mjs --timeout 180000 'dist-esm/test/public/node/*.spec.js' 'dist-esm/test/internal/node/*.spec.js'",


identity's integration tests use the recordings as well, for <reasons>, so we need the test proxy running. Easiest way is to keep using dev-tool commands

Yes I was going to add an option then switch most of our packages over to use that. Eventually also would like to make using the loader default behavior. --no-esm doesn't sound like a great name but I haven't got a better one either.

I was thinking maybe something like "loader" with a default of esm?

Then, you could omit it for existing behavior or pass --loader=esm4mocha for the new mocha loader. Maybe that would be nicer? I'll push that up shortly because I also don't love "no-esm" - let me know how that looks once I push the changes up!

Once we are comfortable we can make the default value esm4mocha and everyone will just get it

I feel that --loader= may be confusing, sounding like you could pass any strings there but they get ignored. How about --use-esm-workaround=false with default value of true? I think there are only a handful packages that doesn't work immediately now with the esm4mocha loader. Once I switch majority over, I can make the default value false then added ``--use-esm-workaround=true` to those a few packages.

Yeah good point and I agree with loader being confusing... let me push up a change with your suggestion and we can see if that reads better 👍

maorleger · 2024-02-14T21:56:55Z

sdk/identity/identity/test/internal/node/clientAssertionCredential.spec.ts

@@ -95,7 +95,6 @@ describe("ClientAssertionCredential (internal)", function () {
      await credential.getToken("https://vault.azure.net/.default");
    } catch (e: any) {
      // We're ignoring errors since our main goal here is to ensure that we send the correct parameters to MSAL.
-      console.log("error", e);


Adds noise, if we're ignoring it let's ignore it

maorleger · 2024-02-14T21:57:19Z

sdk/identity/identity/test/internal/node/clientCertificateCredential.spec.ts

@@ -114,7 +114,7 @@ describe("ClientCertificateCredential (internal)", function () {
  });

  it("throws when given a file that doesn't contain a PEM-formatted certificate", async function (this: Context) {
-    const fullPath = path.resolve(__dirname, "../src/index.ts");
+    const fullPath = path.resolve("./clientCertificateCredential.spec.ts");


__dirname is not supported, but it doesn't actually matter what file we send as long as it's not a proper cert

yeah __dirname is blocking some packages' test too. There's recommendation of const __dirname=path.dirname(url.fileURLToPath(import.meta.url) but import.meta.url requires "module" of "ES2020", which is at odds with our module of commonjs for test:node-ts-input :(

maorleger · 2024-02-14T21:57:52Z

sdk/identity/identity/test/node/msalNodeTestSetup.ts

@@ -46,9 +44,6 @@ export async function msalNodeTestSetup(

  const sandbox = createSandbox();

-  const stub = sandbox.stub(util, "randomUUID");


This is just a better world to live in, no weird wrapping and no concerns with immutable modules

maorleger · 2024-02-14T21:58:04Z

sdk/identity/identity/test/public/node/utils/utils.ts

 import * as net from "net";
 import * as tls from "tls";
+
+import jwt from "jsonwebtoken";


this should be the default export to work

maorleger · 2024-02-14T22:00:26Z

/azp run js - identity - tests

azure-pipelines · 2024-02-14T22:00:40Z

Azure Pipelines successfully started running 1 pipeline(s).

maorleger · 2024-02-14T22:00:56Z

/azp run js - maps-geolocation-rest - tests

maorleger · 2024-02-14T22:01:05Z

/azp run js - communication-identity - tests

azure-pipelines · 2024-02-14T22:01:06Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2024-02-14T22:01:13Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-sdk · 2024-02-14T22:02:58Z

API change check

API changes are not detected in this pull request.

jeremymeng · 2024-02-14T22:44:37Z

common/tools/dev-tool/src/commands/run/testNodeJSInput.ts

  },
 );

 export default leafCommand(commandInfo, async (options) => {
  const reporterArgs =
    "--reporter ../../../common/tools/mocha-multi-reporter.js --reporter-option output=test-results.xml";
  const defaultMochaArgs = `${
-    (await isModuleProject())
+    options["no-esm"] === true || (await isModuleProject())


I was thinking to use an option to toggle between the two workarounds, for example with --no-esm=true and if it is not module project, we would have "--loader=../../../esm4mocha.mjs -r source-map-support/register" so we don't have to pass the loader option in the package's NPM script.

jeremymeng

👍

maorleger added 2 commits February 14, 2024 13:03

[identity] Use custom loader

c27a72e

[identity] Use esm4mocha and re-enable IBC test

0bf4d09

github-actions bot added Azure.Identity Communication - Identity labels Feb 14, 2024

maorleger commented Feb 14, 2024

View reviewed changes

update comm identity

491b8a5

jeremymeng reviewed Feb 14, 2024

View reviewed changes

maorleger added 3 commits February 14, 2024 14:58

better naming for loader?

68c6e2d

reverse order

b5e14c9

revert rush upgrade

f0ca0f2

maorleger linked an issue Feb 14, 2024 that may be closed by this pull request

[Identity] Re-enable interactiveBrowserCredential test #28373

Closed

cleanup

564b60d

maorleger marked this pull request as ready for review February 14, 2024 23:32

maorleger requested review from mikeharder, ckairen, deyaaeldeen, KarishmaGhiya, witemple-msft, schaabs and a team as code owners February 14, 2024 23:32

maorleger requested review from joshfree, pvaneck and billwert February 14, 2024 23:32

maorleger requested review from ahsonkhan and mpodwysocki February 14, 2024 23:32

use-esm-workaround

2b6207b

maorleger force-pushed the esm4mocha-identity branch from ec62363 to 2b6207b Compare February 15, 2024 15:42

jeremymeng approved these changes Feb 15, 2024

View reviewed changes

maorleger merged commit 2e50e2a into Azure:main Feb 15, 2024
23 checks passed

maorleger deleted the esm4mocha-identity branch February 15, 2024 19:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[identity] Use esm4mocha, re-enable IBC test, remove unnecessary stub #28580

[identity] Use esm4mocha, re-enable IBC test, remove unnecessary stub #28580

maorleger commented Feb 14, 2024 •

edited

Loading

maorleger Feb 14, 2024

jeremymeng Feb 14, 2024

maorleger Feb 14, 2024

jeremymeng Feb 14, 2024

maorleger Feb 14, 2024 •

edited

Loading

jeremymeng Feb 15, 2024

maorleger Feb 15, 2024

maorleger Feb 14, 2024

maorleger Feb 14, 2024

jeremymeng Feb 15, 2024

maorleger Feb 14, 2024

maorleger Feb 14, 2024

maorleger commented Feb 14, 2024

azure-pipelines bot commented Feb 14, 2024

maorleger commented Feb 14, 2024

maorleger commented Feb 14, 2024

azure-pipelines bot commented Feb 14, 2024

azure-pipelines bot commented Feb 14, 2024

azure-sdk commented Feb 14, 2024

jeremymeng Feb 14, 2024

jeremymeng left a comment

		@@ -46,9 +44,6 @@ export async function msalNodeTestSetup(

		const sandbox = createSandbox();

		const stub = sandbox.stub(util, "randomUUID");

[identity] Use esm4mocha, re-enable IBC test, remove unnecessary stub #28580

[identity] Use esm4mocha, re-enable IBC test, remove unnecessary stub #28580

Conversation

maorleger commented Feb 14, 2024 • edited Loading

Packages impacted by this PR

Issues associated with this PR

Describe the problem that is addressed by this PR

Provide a list of related PRs (if any)

Command used to generate this PR: (Applicable only to SDK release request PRs)

Checklists

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maorleger Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maorleger commented Feb 14, 2024

azure-pipelines bot commented Feb 14, 2024

maorleger commented Feb 14, 2024

maorleger commented Feb 14, 2024

azure-pipelines bot commented Feb 14, 2024

azure-pipelines bot commented Feb 14, 2024

azure-sdk commented Feb 14, 2024

Choose a reason for hiding this comment

jeremymeng left a comment

Choose a reason for hiding this comment

maorleger commented Feb 14, 2024 •

edited

Loading

maorleger Feb 14, 2024 •

edited

Loading