-
Notifications
You must be signed in to change notification settings - Fork 219
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.
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
Onboard Id Web to Threading Analyzers #3041
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM at first review, @westin-m
Make sure you run end to end tests with credentials.
There was a race condition at some point, which is why the credential loaders used GetAwaiter.GetAsync().
I'm surprised there is no change in the public API?
src/Microsoft.Identity.Web.Certificate/DefaultCertificateLoader.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Identity.Web/AzureADB2COpenIDConnectEventHandlers.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Identity.Web.OWIN/OpenIdConnectCachingSecurityTokenProvider.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Identity.Web/AzureADB2COpenIDConnectEventHandlers.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Identity.Web/AzureADB2COpenIDConnectEventHandlers.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would like to have clarity on which E2E tests were run to verify things are working as expected, and which scenarios were covered.
would like to have no public API changes now.
I've added this to the description |
src/Microsoft.Identity.Web.OWIN/OpenIdConnectCachingSecurityTokenProvider.cs
Outdated
Show resolved
Hide resolved
tests/Microsoft.Identity.Web.Test.Integration/AcquireTokenForAppIntegrationTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConfigureAwait(false) is not compatible with XUnit this is what had been causing our flaky tests previously
remove configureawait from theory test
The following E2E scenarios were tested using the corresponding devapps:
Web app calls web api call graph
B2C web app calls web api
Owin
Blazer server calls web api
Blazer server B2C calls api
Ciam web app calls api
Web app calls graph
Deamon console calling downstream api, graph
All are working as expected.