identity: delete obsolete test

[maorleger]

Packages impacted by this PR

@azure/identity

Issues associated with this PR

N/A - test issue

Describe the problem that is addressed by this PR

When we migrated to MSAL's implementation for Node's IBC in #26724 we discovered
a bug in MSAL's implementation causing an unhandled promise rejection
issue

While this was fixed in MSAL 2.x we are still running tests against MSAL 1.x due
to our ESM package's inability to respect the exports field which MSAL moved
to in 2.x

Unfortunate, and results in our test continuing to crash Mocha. Further, now
that we no longer have our own in-house implementation, this test only tests
that MSAL can throw errors.

At this point, the test is both obsolete and harmful as it continues to cause
unhandled rejections - so I am deleting it.

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

I thought about passing a custom loopbackClient to the test that patches this
behavior but that requires changing source code just to fix a test, more
obsolete code that will be unnecessary once we move to ESM and are unblocked
from running against MSAL's 2.x version

Are there test cases added in this PR? (If not, why?)

Provide a list of related PRs (if any)

AzureAD/microsoft-authentication-library-for-js#6449
#26724

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).

[maorleger]

Some learnings as I was digging into this:

MSAL 2.x and Azure SDK

Tests are failing with:

AuthError [NodeAuthError]: loopback_server_timeout: Timed out waiting for auth code listener to be registered.

Example build

Looking at MSAL's repo I found this issue by Matt, which was fixed in 2.x

Trying to upgrade to MSAL 2.x causes a different error when running integration tests:

Error: Cannot find module '@azure/msal-node'

This also prevents us from upgrading communication-identity's MSAL version to 2.x

So what's going on here (at least as far as I can tell)?

• communication-identity depends on 1.x
• identity depends on 2.x
• 2.x is ESM first with exports field controlling whether CJS or ESM is called
• our dependency, esm ignores exports field and was never updated to understand it
• when identity tests run, they try to find msal-node, but the only CJS they can find is 1.x
• Removing 1.x from our monorepo forces it to try and find 2.x, but is unable to because the esm package does not support it

What can we do?

• Wait for Matt's work on ESM, or support his work to unblock
• Find a workaround to the esm package in the meanwhile
• Ask MSAL to bring back the main field in package.json
• Switch to an ESM fork in the meanwhile: https://www.npmjs.com/package/@httptoolkit/esm as an example

[azure-sdk]

API change check

API changes are not detected in this pull request.

[xirzec]

@maorleger setting aside the matter of how useful this test is, I think if you wanted to unblock testing from being stuck on the esm issue we could migrate to vitest?

[maorleger]

@maorleger setting aside the matter of how useful this test is, I think if you wanted to unblock testing from being stuck on the esm issue we could migrate to vitest?

Would love to learn more, I know we've been moving to vitest in core but I was not aware we can bypass using the esm package when doing so! I can play around with it as an alternative. Would you be open to pending (instead of deleting) this test with an issue to re-enable it once we are able to?

[KarishmaGhiya]

I would be inclined to rewriting this test in some other manner (mocking using another tool and being more efficient) that helps us pick the correct versioning and overcome the complications of cjs bundling, instead of deleting altogether. Reason - because this test actually helped us discover the bug in msal-node's implementation.
Also adding to the list of what we can do:

• I can also reach out to the communication team and check if it's ok to update the dependency on @azure/msal-node. Because they are only using it to get PublicClientApplication interface which is available in 2.x

@maorleger Do you remember if upgrading the msal-node dependency in communication-identity helped us resolve the issue of 1.x getting pulled?

[maorleger]

I would be inclined to rewriting this test in some other manner (mocking using another tool and being more efficient) that helps us pick the correct versioning and overcome the complications of cjs bundling, instead of deleting altogether. Reason - because this test actually helped us discover the bug in msal-node's implementation. Also adding to the list of what we can do:

Yea, totally valid! I am happy to close this PR and find an alternative, let's chat about it today. Maybe deleting this test only kicks the can down the road and what we should do is resolve the incompatibility, maybe by using vitest as Jeff suggested...

• I can also reach out to the communication team and check if it's ok to update the dependency on @azure/msal-node. Because they are only using it to get PublicClientApplication interface which is available in 2.x

It would be good to know but won't help us now. If they had not been using it prior, we would have discovered the incompatibility when we upgraded to 2.x and handled it then but I think at this point communication-identity's dependency on MSAL 1.x is the only thing keeping all live tests from failing (see below).

The unfortunate part is that it means we continue to test against MSAL 1.x whenever we use the esm package (in identity that happens during integration-tests)

@maorleger Do you remember if upgrading the msal-node dependency in communication-identity helped us resolve the issue of 1.x getting pulled?

It did, it resulted in all live test runs failing due to Identity's inability to load MSAL 2.x in tests . @jeremymeng ran into this a few weeks ago #28206 which is how I ended up down the path of trying to understand why we don't even load / find 2.x and discover the bug with the esm package...

[maorleger]

Just to make things much easier to understand I isolated the incompatibility between MSAL 2.x and the esm package - you can see that in this minimal repro

Might be I am missing something super obvious!

[maorleger]

Chatted with Karishma, going to close this PR and meet us halfway with skipping this test (not deleting) and adding an issue for me to dig into alternatives