Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Identity] Re-enable interactiveBrowserCredential test #28373

Closed
maorleger opened this issue Jan 25, 2024 · 2 comments · Fixed by #28580
Closed

[Identity] Re-enable interactiveBrowserCredential test #28373

maorleger opened this issue Jan 25, 2024 · 2 comments · Fixed by #28580
Assignees
@maorleger
Labels
Azure.Identity Client This issue points to a problem in the data-plane of the library. test-reliability Issue that causes tests to be unreliable

Comments

@maorleger
Copy link
Member

maorleger commented Jan 25, 2024 •
edited
Loading

Per learnings below, figure out a way to correctly import MSAL 2.x in integration tests in identity
@maorleger maorleger self-assigned this Jan 25, 2024
@maorleger
Copy link
Member Author

maorleger commented Jan 25, 2024 •
edited
Loading

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: package.json
  • our dependency, esm ignores exports field and was never updated to understand it: bug report
  • 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
  • only integration-tests are failing, as the regular tests use ts-node and typescript sources directly. I dont know why we use esm in one and not the other, so I will need to ask around!

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
  • Use vitest, which may allow us to avoid depending on the esm package entirely

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 maorleger added Client This issue points to a problem in the data-plane of the library. Azure.Identity test-reliability Issue that causes tests to be unreliable labels Jan 25, 2024
@maorleger
Copy link
Member Author

Linking to previous PRs:
#28346
#28374

@maorleger maorleger mentioned this issue Jan 25, 2024
Merged
3 tasks
@maorleger maorleger mentioned this issue Feb 14, 2024
Merged
3 tasks
@maorleger maorleger linked a pull request Feb 14, 2024 that will close this issue
[identity] Use esm4mocha, re-enable IBC test, remove unnecessary stub #28580
Merged
3 tasks
maorleger added a commit that referenced this issue Feb 15, 2024
@maorleger
[identity] Use esm4mocha, re-enable IBC test, remove unnecessary stub (…
2e50e2a 
…#28580)

### Packages impacted by this PR

@azure/identity

### Issues associated with this PR

Resolves #28373 

### Describe the problem that is addressed by this PR

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

1. Re-enables IBC test that was skipped due to #28373
3. Removes sinon stubbing of uuid, replacing it with recorder replacer
4. 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
@github-actions github-actions bot locked and limited conversation to collaborators May 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@maorleger maorleger

Labels
Azure.Identity Client This issue points to a problem in the data-plane of the library. test-reliability Issue that causes tests to be unreliable
Projects
Azure Identity SDK Improvements
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[identity] Use esm4mocha, re-enable IBC test, remove unnecessary stub maorleger/azure-sdk-for-js
1 participant
@maorleger