[Investigate] AcquireTokenSilent_ValidATs_ParallelRequests_Async test seems to fail periodically when running on release builds

[trwalke]

AcquireTokenSilent_ValidATs_ParallelRequests_Async test seems to fail periodically when running on release build.

Logs and Network traces

Error: Assert.Fail failed. Measured performance time EXCEEDED. Max allowed: 2000ms. Elapsed: 2749. AcquireTokenSilent should take roughly 100ms for its internal logic, plus the time needed to access the cache and all the thread context switches

Trace: at Microsoft.Identity.Test.Common.Core.Helpers.PerformanceValidator.Dispose() in \tests\Microsoft.Identity.Test.Common\Core\Helpers\PerformanceValidator.cs:line 30 at Microsoft.Identity.Test.Unit.RequestsTests.ParallelRequestsTests.<AcquireTokenSilent_ValidATs_ParallelRequests_Async>d__9.MoveNext() in \tests\Microsoft.Identity.Test.Unit\ParallelRequestsTests.cs:line 94 --- End of stack trace from previous location where exception was thrown --- at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task) at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task) at System.Runtime.CompilerServices.TaskAwaiter.GetResult() at Microsoft.VisualStudio.TestPlatform.MSTestAdapter.PlatformServices.ThreadOperations.ExecuteWithAbortSafety(Action action)

Performance

Which Version of MSAL are you using ? MSAL 4.24.0

[MichelZ]

using (new PerformanceValidator(
                    NumberOfRequests * (2 * CacheAccessPenaltyMs + 100),

Is this calculation correct? It uses 10 requests, CacheAccessPenaltyMs is 50, that means it currently calculates:

10 * (2 * 50 + 100) = 2000

or should it be: 10 * (2 * (50 + 100)) = 3000 ? NumberOfRequests * (2 * (CacheAccessPenaltyMs + 100)

Where does the 2 * come from? What is it? https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/9fa7dfd18229100a2425aa9176ed124f10077b72/tests/Microsoft.Identity.Test.Unit/ParallelRequestsTests.cs#L79

Also, on the AcquireTokenSilent_ExpiredATs_ParallelRequests_Asynctest, while the text says 100ms for its internal logic, the test code actually allows 1000ms. Bug? https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/9fa7dfd18229100a2425aa9176ed124f10077b72/tests/Microsoft.Identity.Test.Unit/ParallelRequestsTests.cs#L154

[MichelZ]

@trwalke @bgavrilMS what do you think? Shall I PR this?

[bgavrilMS]

I think we should delete this test because we have better performance tests, especially around the cache.

Afair, the 2 comes from the fact that a token request makes 2 cache accesses (one for read, one for write)