integ testig + bug fixes #1304

SomkaPe · 2018-10-04T23:50:49Z

while writing integration tests next bugs were found and fixed:

Clear cache API - on Msal clear cache api did not clear persistent version of Adal cache because "onAfter" delegate was not called (only in memory representation was cleared).
as result adal cache was not cleared.
TokenCache.Clear internal Api does not clear persistent version of Adal cache microsoft-authentication-library-for-dotnet#651
Same problem with opposite direction Adal -> Msal.
TokenCache.Clear public Api does not clear persistent version of Msal cache #1344
RemoveAdalUser api should remove users by accountId and by displayableId ( to cover scenario of
migrarion form adalv3 to adalv4 to msal2)
RemoveAdalUser internal API does not remove all relevant Adal cache entities (migrarion form adalv3 to adalv4 to msal2 scenario) microsoft-authentication-library-for-dotnet#652
NullPoinetEx in RemoveMsalAccount (Msal) - when adalv3 account passed
NullPoinetEx in RemoveAdalUser (Msal) - when adalv3 account passed
NullPoinetEx in RemoveMsalAccount and RemoveAdalUser when adalv3 account passed microsoft-authentication-library-for-dotnet#653
TokenCacheSerializeHelper.DeserializeUnifiedCache did not clear in memory version of cache so it was appending it
TokenCacheSerializeHelper.DeserializeUnifiedCache does not clear in memory version of cache Instead int appends it microsoft-authentication-library-for-dotnet#654

bgavrilMS · 2018-10-05T08:28:32Z

msal/tests/Test.MSAL.NET.Integration/DeviceCodeIntegrationTests.cs

@@ -47,6 +47,7 @@ public class DeviceCodeIntegrationTests

        [TestMethod]
        [TestCategory("UsernamePasswordIntegrationTests")]
+        [Ignore]


I don't think this is needed, they should pass on the build machine as the user running the tests has access to KeyVault. @jennyf19 - the tests should fail more gracefully, i.e. they fail with a NullRef today, they should fail with a nice message #Closed

this test more some kind of manual test since the step of authentication is not automated, so it is not integration test #Closed

pls. do not ignore tests. if it doesn't pass locally due to missing setup investigate how to best fix that.

In reply to: 222927993 [](ancestors = 222927993)

it does not work because authentication is not implemented, so it is not automated test #Closed

UsernamePassword is implemented and shipped on MSAL since 2.1.0. We even shipped Device Flow in 2.2.0 :). This test passes on the build machine, but fails on our machines. If you just delete the [Ignore] I am sure it will not cause issues on the build machine. We will look separately into making it pass or at least fail more gracefully on dev machines. #Closed

it can not pass anywhere it is not even test , it makes no sense, it just some random copy-paste(test category, test name is not related to device code flow, user object is not used , empty call back is passed)
It just wait 15 mins and than fails (because deviceCode result is just ignored - empty callback passed).

If you think it pass on build machine It just means that it is not get run there.

#Closed

If I understood correctly, this is a copy paste error. Please delete the test.

In general we should not [Ignore] tests because we forget about them and after a few months they become dead code. We should only Ignore if we create a bug / work item to track the work of un-ignoring. #Closed

@MarkZuber : What was the intentions of this? Should it be deleted? How is the e2e of Device Codeflow validated now? Manually (does the test exist in VSTS)?

In reply to: 224132337 [](ancestors = 224132337)

This was put in early in development of devicecode and wasn't fully fleshed out because we can't automate the browser side of things. so i'll kill this test for now. yes, Henrik, for now this test is manual since we don't automate the browser.

leave it as ignore in your PR (it's ignore on my end too) and i'll kill it with my refactor PR. #Closed

bgavrilMS · 2018-10-05T08:32:41Z

core/src/Cache/CacheFallbackOperations.cs

-                    RemoveEntriesWithMatchingName(environmentAliases, clientId, displayableId, adalCache);
-                }
+
+                RemoveEntriesWithMatchingName(environmentAliases, clientId, displayableId, adalCache);


Could you explain, and also add a comment in the code - how will this help the ADAL v3 -> ADAL v4 -> MSAL upgrade?

Also, don't you think we will be deleting too much in the standard case? The RemoveEntriesWithMatchingName will delete entries with the same name even if they are from different accounts ...

#Closed

this case is covered in UnifiedCache_Adalv3ToAdalV4ToMsal2MigrationIntegrationTest :
if you get AT for res1 in adal v3 than migrate to adal v4 and get token to res2 you will have next state of the cache:
for the same Account you have one entity in adal cache in adalv3 format (no client info)
another entity in adal v4 format (with client info).
So you have two Adal cache entities for the same account .

If you migrate to Msal and call get accounts api one Account object will be returned.
If you call Remove account it will remove only V4 adal entity , so next call to get accounts will return same account object (this time with id null)

So remove account api will not remove account .
Unless remove account api remove by displayble id also.

#Closed

We have been going back and forth on this. Last time we ended up saying we should not do both deletes as it have side effects.

In reply to: 223143317 [](ancestors = 223143317)

not sure what side effects you mean here , In extremely rare case when few different accounts with same displaybleId/preferred_user_name were used in Adal, Msal remove Account api will remove SSO in adal cache format for both of them which is less evil that not working removeAccount api for described scenario(which is quite possible scenario ) . If you want me to remove this change I can do it. #Closed

I understand the scenario now, thank you for explaining Petro. Definitely needs to be in the code as a comment if we decided to go for it. Is this algorithm discussed with the other teams or is it only us that have this problem? #Closed

Did you cover this with tests Petro? Is that the test you referred to in another comment?

In reply to: 224130526 [](ancestors = 224130526)

@bgavrilMS, are you ok with this change?

In reply to: 224675645 [](ancestors = 224675645,224130526)

@SomskaPe, are the tests in place for this?

In reply to: 226489008 [](ancestors = 226489008,224675645,224130526)

Yes, sounds good. Better to delete more than to not be able to remove accounts from cache...

In reply to: 226489100 [](ancestors = 226489100,226489008,224675645,224130526)

I have added test to CacheFallbackOperationsTests, But I got puzzled after quick look at tests in that class,
@bgavrilMS Why PopulateLegacyCache function is shared by many tests ? some tests call it and add more entities to the cache - so state of the cache is messed up.
in order to test logic in CacheFallbackOperations you need to consciously configure cache with some state, describe that state Than run some logic and check expectations.
I mean business logic completely depends on the state of the cache,
so it is crucial incoming parameter to any test. It can not be just some shared random entities
#Closed

bgavrilMS · 2018-10-05T08:37:15Z

core/src/Cache/TokenCacheSerializeHelper.cs

@@ -59,6 +59,8 @@ internal static void DeserializeUnifiedCache(TokenCacheAccessor tokenCacheAccess
                return;
            }

+            tokenCacheAccessor.Clear();


Shouldn't this go to before if (cacheDict is empty) ? #Closed

oh, right - will fix it #Closed

Good Catch #Closed

bgavrilMS · 2018-10-05T08:41:05Z

adal/src/Microsoft.IdentityModel.Clients.ActiveDirectory/InternalsVisibleTo.cs

@@ -34,3 +34,4 @@
 [assembly: InternalsVisibleTo("Test.ADAL.NET.Unit, PublicKey=0024000004800000940000000602000000240000525341310004000001000100B5FC90E7027F67871E773A8FDE8938C81DD402BA65B9201D60593E96C492651E889CC13F1415EBB53FAC1131AE0BD333C5EE6021672D9718EA31A8AEBD0DA0072F25D87DBA6FC90FFD598ED4DA35E44C398C454307E8E33B8426143DAEC9F596836F97C8F74750E5975C64E2189F45DEF46B2A2B1247ADC3652BF5C308055DA9")]
 [assembly: InternalsVisibleTo("Test.ADAL.NET.Integration, PublicKey=0024000004800000940000000602000000240000525341310004000001000100B5FC90E7027F67871E773A8FDE8938C81DD402BA65B9201D60593E96C492651E889CC13F1415EBB53FAC1131AE0BD333C5EE6021672D9718EA31A8AEBD0DA0072F25D87DBA6FC90FFD598ED4DA35E44C398C454307E8E33B8426143DAEC9F596836F97C8F74750E5975C64E2189F45DEF46B2A2B1247ADC3652BF5C308055DA9")]
 [assembly: InternalsVisibleTo("WinFormsAutomationApp, PublicKey=0024000004800000940000000602000000240000525341310004000001000100B5FC90E7027F67871E773A8FDE8938C81DD402BA65B9201D60593E96C492651E889CC13F1415EBB53FAC1131AE0BD333C5EE6021672D9718EA31A8AEBD0DA0072F25D87DBA6FC90FFD598ED4DA35E44C398C454307E8E33B8426143DAEC9F596836F97C8F74750E5975C64E2189F45DEF46B2A2B1247ADC3652BF5C308055DA9")]
+[assembly: InternalsVisibleTo("Test.MSAL.NET.Integration, PublicKey=00240000048000009400000006020000002400005253413100040000010001002D96616729B54F6D013D71559A017F50AA4861487226C523959D1579B93F3FDF71C08B980FD3130062B03D3DE115C4B84E7AC46AEF5E192A40E7457D5F3A08F66CEAB71143807F2C3CB0DA5E23B38F0559769978406F6E5D30CEADD7985FC73A5A609A8B74A1DF0A29399074A003A226C943D480FEC96DBEC7106A87896539AD")]


These are for the L1 tests, i.e. E2E headless. If you need internal relation, these are L0, and they should go to the unit test assembly. #Closed

not sure what you mean here, please explain #Closed

The basic idea is that integration tests should have no reason to have access to internals. If you need access to internals then perhaps the test is a unit test and not an integration test.

In reply to: 223127605 [](ancestors = 223127605)

how would you check state of the cache or modify it (for example to expire AT to trigger RT usage)? #Closed

You don't. Unit tests do that. #Closed

This is integration testing of msal and adal coexistence it is not unit test #Closed

which is exactly why there should ideally not be a reference... but makes sense.

In reply to: 224193062 [](ancestors = 224193062)

end to end test does not need access to internals, integration test can have access to it, discussed project called "Test.MSAL.NET.Integration" #Closed

bgavrilMS · 2018-10-05T08:47:08Z

msal/tests/Test.MSAL.NET.Integration/Test.MSAL.NET.Integration.csproj

@@ -4,6 +4,9 @@
    <TargetFramework>net4.5.2</TargetFramework>

    <IsPackable>false</IsPackable>
+
+    <SignAssembly>true</SignAssembly>


Good idea, we should sign all our test assemblies instead of applying the sn hack #Closed

can this be applied to all test and sample projects?

In reply to: 222932902 [](ancestors = 222932902)

bgavrilMS · 2018-10-05T08:48:14Z

msal/tests/Test.MSAL.NET.Integration/Test.MSAL.NET.Integration.csproj

@@ -19,8 +22,13 @@
  </ItemGroup>

  <ItemGroup>
+    <ProjectReference Include="..\..\..\adal\src\Microsoft.IdentityModel.Clients.ActiveDirectory\Microsoft.IdentityModel.Clients.ActiveDirectory.csproj">


Please add a comment on why this is needed, i.e. qualified name collisions... #Closed

to cover unified cache test cases we need to be able create instances of Adal and Msal in same test #Closed

Sorry for not making myself clear. When I say "please add a comment", I mean "please add a comment in the code" not "please add a comment in the PR" #Closed

I'm still confused as to why this is all needed. Is it because you can' t access core elements? ... is there a trick for this in the using statements, perhaps not but was hoping.

In reply to: 223487288 [](ancestors = 223487288)

bgavrilMS · 2018-10-05T08:49:58Z

msal/tests/Test.MSAL.NET.Unit/AuthorityAliasesTests.cs

+                    TestConstants.GetDiscoveryEndpoint(TestConstants.AuthorityCommonTenant)));
+        }
+
+        class TestLegacyCachePersistance : ILegacyCachePersistance


This class also exists in UnifiedCacheTests - could you pull it out into the Mocks directory and use it from there? Alternatively, use NSubstitute to create on the fly. #Closed

ok, will change #Closed

bgavrilMS · 2018-10-05T08:53:21Z

msal/tests/Test.MSAL.NET.Unit/AuthorityAliasesTests.cs

+                }
+                catch (AggregateException ex)
+                {
+                    Assert.IsNotNull(ex.InnerException);


The intent of the test is not clear - do we expect a UIRequiredException or do we expect the result to be retured? Please refactor so that the requests that result in an exception fail if a result is returned and vice versa. #Closed

good catch, will change #Closed

bgavrilMS · 2018-10-05T09:01:55Z

core/src/Cache/CacheFallbackOperations.cs

-                    RemoveEntriesWithMatchingName(environmentAliases, clientId, displayableId, adalCache);
-                }
+
+                RemoveEntriesWithMatchingName(environmentAliases, clientId, displayableId, adalCache);


There are unit tests for this logic, if changes are made, there should be unit test changes ( a bit surprised if they do no fail) #Closed

This is covered by UnifiedCache_Adalv3ToAdalV4ToMsal2MigrationIntegrationTest #Closed

bgavrilMS

🕐

MarkZuber · 2018-10-05T16:22:03Z

msal/tests/Test.MSAL.NET.Integration/UsernamePasswordIntegrationTests.cs


 using Test.Microsoft.Identity.LabInfrastructure;
-using Microsoft.Identity.Client;
+using msal::Microsoft.Identity.Client;


can you please explain why this msal:: alias is now needed? #Closed

because of namespace collisions between adal and msal, I have added integ tests for unified cache which create msal and adal client in the same test #Closed

Yeah, I have seen this as well. DeviceCodeResult now exists in 2 separate assemblies (ADAL and MSAL) and the current assemblies references both. As such it is a separate type, even though the fully qualified name (namespace + type name) is the same. So .net needs a way to disambiguate between the 2.

I have a work in progress to get rid of "core" unit tests and just have the core tests run as part of ADAL and as part of MSAL. This will eliminate the need for assembly aliasing. Until then, it's probably the only way. #Closed

henrik-me · 2018-10-08T15:50:58Z

adal/src/Microsoft.IdentityModel.Clients.ActiveDirectory/TokenCache.cs

@@ -294,22 +294,24 @@ public virtual void Clear()
        {
            lock (cacheLock)
            {
+                TokenCacheNotificationArgs args = new TokenCacheNotificationArgs { TokenCache = this };
+                this.OnBeforeAccess(args);


is it needed to have the lock on the cache for the onbeforeaccess?

Alos the cache is also locked for every call to the eventlisteners. #Closed

probably no , in general onBeforeAccess call is not necessary here , we do not need to read it here.
But It is general pattern of accessing cache here (in this lib) #Pending

would have liked to see if we could loosen this a bit and have fewer locks

In reply to: 223477053 [](ancestors = 223477053)

fewer locks ? There is only one lock for cache. Did you mean shorter critical sections ? Probably yes.
This deserves its own user story and its own pr. #Pending

henrik-me · 2018-10-08T15:57:21Z

core/src/Cache/TokenCacheSerializeHelper.cs

@@ -50,6 +50,8 @@ internal class TokenCacheSerializeHelper
        /// <param name="requestContext">call state to pass correlation id and logger instance</param>
        internal static void DeserializeUnifiedCache(TokenCacheAccessor tokenCacheAccessor, byte[] unifiedState, RequestContext requestContext)
        {
+            tokenCacheAccessor.Clear();


tokenCacheAccessor [](start = 12, length = 18)

This seems important (an alternative is to throw if the tokenAccessor is not empty, or to simply create the tokenCacheAccessor in the helper method).... are unit tests covering this helper's behavior? #Pending

Why would you throw here ? #Closed

I'm suggesting 2 ways to deal with this differently, to identify the better of these options or potentially a third I would need to understand why you had to add this in the first place.

To me it's magic that we decide to clear the object, and if we do there is really no need to pass it in. We can just return a new accessor instead.

Alternatively if it's very important that the object has been cleared in advance then we can throw an exception instead (should the object not be in the expected state coming in)

How did you discover that .clear was needed?
#Pending

For custom cache persistence scenarios, every cache access preceded by before access delegate call, which is the place were cache supposed to be read from some persistent storage and deserialized to in-memory representation.
This is important to reflect current state of the cache, persistent state of cache is "golden source" of it , not in-memory version.
For example :

you share cache between few app

in app1 you create msalClient1, then acquire token (now you have tokens in the cache).

switch to app2 create msalClient2 then clear cache

return to app1 and execute getAccount on msalClient1 - during deserialization in memory representation of the cache should be cleared and instead persistent state (which is empty now) should be used,
#Closed

henrik-me · 2018-10-08T15:57:58Z

core/tests/Test.Microsoft.Identity.Core.Unit/Mocks/MockHelpers.cs

+            {
+                data = serializedCache;
+            }
+        }


new file #Closed

Please place this class in a new file. See other objects such as the TestLogger.cs #Closed

henrik-me · 2018-10-08T15:59:15Z

msal/src/Microsoft.Identity.Client/TokenCache.cs

@@ -901,6 +901,11 @@ internal async Task RemoveAsync(string authority, bool validateAuthority, IAccou

        internal void RemoveMsalAccount(IAccount account, ISet<string> environmentAliases, RequestContext requestContext)
        {
+            if (account.HomeAccountId == null)


if (account.HomeAccountId == null) [](start = 12, length = 34)

is this properly covered by utests?
Also should probably at least be logged as info? #Closed

Adal v3 to Msal v2 migration is not covered by unit test, this integration project is only one which has references to adal an msal so this scenario can be tested #Closed

henrik-me · 2018-10-08T16:00:58Z

msal/src/Microsoft.Identity.Client/Properties/InternalsVisibleTo.cs

+[assembly: InternalsVisibleTo("Test.MSAL.NET.Unit,       PublicKey=00240000048000009400000006020000002400005253413100040000010001002D96616729B54F6D013D71559A017F50AA4861487226C523959D1579B93F3FDF71C08B980FD3130062B03D3DE115C4B84E7AC46AEF5E192A40E7457D5F3A08F66CEAB71143807F2C3CB0DA5E23B38F0559769978406F6E5D30CEADD7985FC73A5A609A8B74A1DF0A29399074A003A226C943D480FEC96DBEC7106A87896539AD")]
+[assembly: InternalsVisibleTo("Test.MSAL.NET.Integration, PublicKey=00240000048000009400000006020000002400005253413100040000010001002D96616729B54F6D013D71559A017F50AA4861487226C523959D1579B93F3FDF71C08B980FD3130062B03D3DE115C4B84E7AC46AEF5E192A40E7457D5F3A08F66CEAB71143807F2C3CB0DA5E23B38F0559769978406F6E5D30CEADD7985FC73A5A609A8B74A1DF0A29399074A003A226C943D480FEC96DBEC7106A87896539AD")]


InternalsVisibleTo("Test.MSAL.NET.Integration [](start = 11, length = 45)

see related comment. #Pending

As I replied in prev comments Integration test can and in our case absolutely need access to internals. End to End test generally should not. #Closed

henrik-me · 2018-10-08T16:01:53Z

msal/tests/Test.MSAL.NET.Integration/DeviceCodeIntegrationTests.cs

@@ -25,8 +25,10 @@
 //
 //------------------------------------------------------------------------------

+extern alias msal;


not needed. you can specify alias'es as art of the using statement #Closed

henrik-me · 2018-10-08T16:03:53Z

msal/tests/Test.MSAL.NET.Integration/UnifiedCacheTests.cs

+//------------------------------------------------------------------------------
+
+extern alias msal;
+extern alias adal;


add these alias's to the using statements instead. #Closed

henrik-me · 2018-10-08T16:36:37Z

msal/tests/Test.MSAL.NET.Unit/TestConstants.cs

+        public static readonly string Uid = "my-uid";
+        public static readonly string Utid = "my-utid";
+
+        public static readonly string ProdPrefNetworkEnv = "login.microsoftonline.com";


not sure I understand the need for shortening the consts. Please change it to meaningful names. reading the test code I found it hard to understand these abbreviations #Pending

ok, will change back #Closed

Seems like these are still not changed.

In reply to: 223459786 [](ancestors = 223459786)

henrik-me · 2018-10-08T16:40:07Z

msal/tests/Test.MSAL.NET.Unit/AuthorityAliasesTests.cs

+
+            foreach (KeyValuePair<AdalTokenCacheKey, AdalResultWrapper> kvp in adalCache)
+            {
+                Assert.AreEqual(expectedEnvironment, new Uri(kvp.Key.Authority).Host);


new Uri(kvp.Key.Authority).Host [](start = 53, length = 31)

nit: would be great if it was not required to new up an object. #Closed

i need to extract host form url string here #Closed

henrik-me · 2018-10-08T16:40:32Z

msal/tests/Test.MSAL.NET.Unit/AuthorityAliasesTests.cs

+                Assert.AreEqual(expectedEnvironment, id.Environment);
+            }
+
+            var accounts = cache.GetAllAccounts(new RequestContext(new MsalLogger(Guid.NewGuid(), null)));


new MsalLogger(Guid.NewGuid(), null) [](start = 67, length = 36)

can you use the same logger for all? What about the same request context? #Pending

+1; #Pending

On top of this, we should check that no errors are logged. This is quite important because a lot of the token cache logic does a try / catch / ignore exception and log, so it's the only way to tell if an exception was thrown... #Pending

@SomskaPe ?

In reply to: 223489018 [](ancestors = 223489018)

You can setup a logger stub using NSubstitute and if anything is logged via Error or ErrorPii, fail the test. We do smth similar here https://github.com:443/AzureAD/azure-activedirectory-library-for-dotnet.git/blob/dev/core/tests/Test.Microsoft.Identity.Core.Unit/CacheTests/CacheFallbackOperationsTests.cs#L188

In reply to: 224676750 [](ancestors = 224676750,223489018)

https://github.com/AzureAD/azure-activedirectory-library-for-dotnet/blob/dev/core/tests/Test.Microsoft.Identity.Core.Unit/CacheTests/CacheFallbackOperationsTests.cs#L188

In reply to: 224727703 [](ancestors = 224727703,224676750,223489018)

@SomskaPe ?

In reply to: 224780601 [](ancestors = 224780601,224727703,224676750,223489018)

All your integration tests invoke logic from the CacheFallbackOperations class. All the methods in this class follow the pattern try { ... } catch all exception and log them. As such, all your integration tests should check that no errors or warnings are ever logged.

In reply to: 226488693 [](ancestors = 226488693,224780601,224727703,224676750,223489018)

henrik-me · 2018-10-08T16:44:37Z

msal/tests/Test.MSAL.NET.Integration/DeviceCodeIntegrationTests.cs

@@ -45,6 +47,7 @@ public class DeviceCodeIntegrationTests

        [TestMethod]
        [TestCategory("UsernamePasswordIntegrationTests")]
+        [Ignore]
        public async Task AcquireTokenWithManagedUsernamePasswordAsync()


AcquireTokenWithManagedUsernamePasswordAsync [](start = 26, length = 44)

@jennyf19: Not sure why this test is in a test class for DeviceCodeIntegrationTests, thoughts? #Closed

Not sure. The actual u/p integration tests are in a separate class - I think the method name and TestCategory name did not get changed on a copy/paste. just a guess. want me to fix it?

In reply to: 223430145 [](ancestors = 223430145)

copy paste == evil #Closed

henrik-me · 2018-10-10T16:15:45Z

msal/tests/Test.MSAL.NET.Integration/UnifiedCacheTests.cs

+            Assert.AreEqual(1, msalAccounts.Count());
+
+            msalAuthResult = await msalPublicClient.AcquireTokenSilentAsync(MsalScopes, msalAccounts.First());
+            ValidateMsalAuthResult();


I would suggest using the same with ADAL, clearing the ADAL cache first? #Closed

henrik-me · 2018-10-10T16:18:08Z

msal/tests/Test.MSAL.NET.Integration/UnifiedCacheTests.cs

+                new adal::Microsoft.IdentityModel.Clients.ActiveDirectory.UserPasswordCredential(user.Upn, securePassword));
+            ValidateAdalAuthResult();
+
+            // simulate adalV3 token cache state by setting client info in adal cache entities to null 


simulate adalV3 token cache state by setting client info in adal cache entities to null [](start = 15, length = 88)

nice workaround :) #Closed

henrik-me · 2018-10-10T16:21:31Z

msal/tests/Test.MSAL.NET.Integration/UnifiedCacheTests.cs

+            adalAuthResult = await adal::Microsoft.IdentityModel.Clients.ActiveDirectory.AuthenticationContextIntegratedAuthExtensions.
+                AcquireTokenAsync(adalContext, AdalResource2, ClientId,
+                new adal::Microsoft.IdentityModel.Clients.ActiveDirectory.UserPasswordCredential(user.Upn, securePassword));
+            ValidateAdalAuthResult();


suggestion before this add a silent call using adal v4 to the resource you already requested an AT for... that way you will be able to validate that v4 works even with-out user_info... or perhaps that it doesn't work (expected?) #Pending

good point will add #Closed

henrik-me · 2018-10-10T16:23:49Z

msal/tests/Test.MSAL.NET.Integration/UnifiedCacheTests.cs

+            Assert.IsNotNull(account.HomeAccountId);
+            Assert.IsNotNull(account.Environment);
+
+            // make sure that adal v4 RT is usable by Msal


// make sure that adal v4 RT is usable by Msal [](start = 12, length = 46)

Not sure it is needed as you have already validted this in other tests, however would it make sense to delete the msal cache first here as well? #Closed

i think no - it is covered in prev tests #Closed

henrik-me · 2018-10-10T16:24:55Z

msal/tests/Test.MSAL.NET.Integration/UnifiedCacheTests.cs

+            // make sure Msal remove account api remove corresponding cache entities in all formats  
+            msalAccounts = await msalPublicClient.GetAccountsAsync();
+            Assert.AreEqual(1, msalAccounts.Count());
+            account = msalAccounts.First();


consider validating the number of cache items as well in each cache #Pending

henrik-me · 2018-10-10T16:25:19Z

msal/tests/Test.MSAL.NET.Integration/UnifiedCacheTests.cs

+            ValidateAdalAuthResult();
+
+            // At this poing Adal cache contains RTs for the same account in diff format v3 and v4
+            Assert.IsTrue(adalCache.ReadItems().Count() == 2);


Assert.IsTrue(adalCache.ReadItems().Count() == 2); [](start = 12, length = 50)

perhaps validate msal cache as well? (I assume one entry will be there) #Closed

it i actually validated in next lines by calling getAccounts api #Closed

henrik-me

🕐

trwalke · 2018-10-11T13:39:18Z

msal/tests/Test.MSAL.NET.Integration/UnifiedCacheTests.cs

+        {
+            await AcquireTokensUsingMsal();
+
+            ClearAdalCache();


Why clear the ADAL cache here? #Closed

trwalke · 2018-10-11T13:42:27Z

msal/tests/Test.MSAL.NET.Integration/UnifiedCacheTests.cs

+        {
+            await AcquireTokensUsingAdal();
+
+            ClearMsalCache();


Why clear MSAL cache in ADAL cache test? #Closed

because adal v4 and msal v2 now share sso - so can read and write each other format #Closed

henrik-me · 2018-10-11T15:48:11Z

msal/tests/Test.MSAL.NET.Integration/UnifiedCacheTests.cs

+            Assert.IsNotNull(account.HomeAccountId);
+            Assert.IsNotNull(account.Environment);
+
+            // validate than Adal writes only Rt in Msal format  an


format an [](start = 57, length = 10)

this text update doesn't seem right... something is missing #Closed

fixed #Closed

henrik-me · 2018-10-11T15:50:06Z

msal/tests/Test.MSAL.NET.Integration/UnifiedCacheTests.cs

+        private SecureString securePassword;
+
+        private byte[] AdalV3StateStorage;
+        private byte[] UnifiedStateStorage;


you will need to also have these ones in each test to ensure they can run completely independent of each other. for the token persistance handling, you can likely use delegates in each test. #Pending

for every test method new instance of test class is instantiated, so it is not shared #Pending

Not sure about that... should still be a fairly easy change.

In reply to: 224570927 [](ancestors = 224570927)

I am pretty sure about it - it is not shared #Closed

henrik-me · 2018-10-11T15:51:12Z

msal/tests/Test.MSAL.NET.Integration/UnifiedCacheTests.cs

+        [TestInitialize]
+        public void TestInitialize()
+        {
+            user = authHelper.GetUser(


nit: you can probably do this only if null, the first test that runs will set it.
@trwalker, have you added std. users to the test helper? #Pending

ok, will do it in static constructor #Pending

henrik-me · 2018-10-11T15:51:40Z

msal/tests/Test.MSAL.NET.Integration/UnifiedCacheTests.cs

+                    IsFederatedUser = false
+                });
+
+            securePassword = new NetworkCredential("", ((LabUser)user).GetPassword()).SecurePassword;


nit: likely only need to set this once, thus if already set it should be good. #Pending

ok, will do it in static constructor #Pending

henrik-me · 2018-10-18T22:46:33Z

Still there are a number of unaddressed comments in the tests, I marked those as pending. @SomskaPe, kindly answer the still active questions: there are 2 open items to close before this PR can move on. #Closed

henrik-me · 2018-10-19T17:28:13Z

@SomkaPe pls. do the integration and ensure the PR is ready to commit. The most important issues has been addressed. #Closed

…y-library-for-dotnet into somkape/integT

SomkaPe · 2018-10-19T23:32:52Z

@SomkaPe pls. do the integration and ensure the PR is ready to commit. The most important issues has been addressed.

@bgavrilMS
@henrik-me
It is ready to commit.
I merged with dev, and added test case to CacheFallbackOperationsTests.

Not sure how much more time you want to spend on this PR but please pay attention to the fact that all described bugs are pretty severe prod bugs.

If you wait for some more changes please notify me and describe very specifically what changes you expect (but first check recent commits)

SomkaPe · 2018-10-22T20:31:31Z

@bgavrilMS
@henrik-me
Any update on this ? #Closed

henrik-me · 2018-10-22T21:32:52Z

Please ensure you document what you consider product bugs as issues. You should link all the issues to this PR.

In reply to: 431527007 [](ancestors = 431527007)

henrik-me · 2018-10-22T21:33:04Z

reviewing again now.

In reply to: 431973200 [](ancestors = 431973200)

henrik-me

SomkaPe · 2018-10-22T22:37:15Z

Please ensure you document what you consider product bugs as issues. You should link all the issues to this PR.

In reply to: 431527007 [](ancestors = 431527007)

Created corresponding issues and added to pr description

SomkaPe added 3 commits October 4, 2018 16:50

integ testig + bug fixes

0b85096

minor fix

8be6107

add ignore attr to not working integ test - device code flow

de4d6e5

AzureAD deleted a comment from jennyf19 Oct 5, 2018

SomkaPe requested review from jennyf19 and henrik-me October 5, 2018 01:04

bgavrilMS reviewed Oct 5, 2018

View reviewed changes

bgavrilMS suggested changes Oct 5, 2018

View reviewed changes

MarkZuber reviewed Oct 5, 2018

View reviewed changes

addressing pr comments

ce57e02

SomkaPe requested a review from trwalke October 5, 2018 22:56

henrik-me reviewed Oct 8, 2018

View reviewed changes

henrik-me reviewed Oct 10, 2018

View reviewed changes

henrik-me suggested changes Oct 10, 2018

View reviewed changes

SomkaPe added 2 commits October 10, 2018 17:04

addressing pr comments

31f9636

minor fix

07920f0

trwalke reviewed Oct 11, 2018

View reviewed changes

henrik-me reviewed Oct 11, 2018

View reviewed changes

SomkaPe added 3 commits October 11, 2018 12:26

code review comments

49f1c0e

Merge branch 'dev' into somkape/integT

2c4646b

test fix

e8a700e

SomkaPe added 2 commits October 19, 2018 11:15

Merge branch 'dev' of https://github.com/AzureAD/azure-activedirector…

05b0f70

…y-library-for-dotnet into somkape/integT

merging with dev, adding unit test to CacheFallBackOperationsTests

e5130c6

henrik-me approved these changes Oct 22, 2018

View reviewed changes

SomkaPe merged commit 7c4c51b into dev Oct 22, 2018

SomkaPe deleted the somkape/integT branch December 13, 2018 18:37

		[assembly: InternalsVisibleTo("Test.MSAL.NET.Unit, PublicKey=00240000048000009400000006020000002400005253413100040000010001002D96616729B54F6D013D71559A017F50AA4861487226C523959D1579B93F3FDF71C08B980FD3130062B03D3DE115C4B84E7AC46AEF5E192A40E7457D5F3A08F66CEAB71143807F2C3CB0DA5E23B38F0559769978406F6E5D30CEADD7985FC73A5A609A8B74A1DF0A29399074A003A226C943D480FEC96DBEC7106A87896539AD")]
		[assembly: InternalsVisibleTo("Test.MSAL.NET.Integration, PublicKey=00240000048000009400000006020000002400005253413100040000010001002D96616729B54F6D013D71559A017F50AA4861487226C523959D1579B93F3FDF71C08B980FD3130062B03D3DE115C4B84E7AC46AEF5E192A40E7457D5F3A08F66CEAB71143807F2C3CB0DA5E23B38F0559769978406F6E5D30CEADD7985FC73A5A609A8B74A1DF0A29399074A003A226C943D480FEC96DBEC7106A87896539AD")]

integ testig + bug fixes #1304

integ testig + bug fixes #1304

Conversation

SomkaPe commented Oct 4, 2018 • edited Loading

bgavrilMS Oct 5, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

SomkaPe Oct 5, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SomkaPe Oct 8, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

bgavrilMS Oct 8, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

SomkaPe Oct 8, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

bgavrilMS Oct 10, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

henrik-me Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

MarkZuber Oct 10, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

bgavrilMS Oct 5, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

SomkaPe Oct 5, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SomkaPe Oct 8, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

bgavrilMS Oct 8, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

henrik-me Oct 12, 2018 • 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

SomkaPe Oct 19, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

bgavrilMS Oct 5, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

SomkaPe Oct 5, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

SomkaPe Oct 5, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

bgavrilMS Oct 5, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

SomkaPe Oct 5, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

henrik-me Oct 8, 2018 • edited Loading

Choose a reason for hiding this comment

SomkaPe Oct 8, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

bgavrilMS Oct 10, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

SomkaPe Oct 10, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SomkaPe Oct 19, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

bgavrilMS Oct 5, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgavrilMS Oct 5, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

SomkaPe Oct 5, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

bgavrilMS Oct 8, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgavrilMS Oct 5, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

SomkaPe Oct 5, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

bgavrilMS Oct 5, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

SomkaPe Oct 5, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

bgavrilMS Oct 5, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

SomkaPe Oct 5, 2018 • edited by henrik-me Loading

Choose a reason for hiding this comment

SomkaPe commented Oct 4, 2018 •

edited

Loading

bgavrilMS Oct 5, 2018 •

edited by henrik-me

Loading

SomkaPe Oct 5, 2018 •

edited by henrik-me

Loading

SomkaPe Oct 8, 2018 •

edited by henrik-me

Loading

bgavrilMS Oct 8, 2018 •

edited by henrik-me

Loading

SomkaPe Oct 8, 2018 •

edited by henrik-me

Loading

bgavrilMS Oct 10, 2018 •

edited by henrik-me

Loading

henrik-me Oct 10, 2018 •

edited

Loading

MarkZuber Oct 10, 2018 •

edited by henrik-me

Loading

bgavrilMS Oct 5, 2018 •

edited by henrik-me

Loading

SomkaPe Oct 5, 2018 •

edited by henrik-me

Loading

SomkaPe Oct 8, 2018 •

edited by henrik-me

Loading

bgavrilMS Oct 8, 2018 •

edited by henrik-me

Loading

henrik-me Oct 12, 2018 •

edited

Loading

SomkaPe Oct 19, 2018 •

edited by henrik-me

Loading

bgavrilMS Oct 5, 2018 •

edited by henrik-me

Loading

SomkaPe Oct 5, 2018 •

edited by henrik-me

Loading

SomkaPe Oct 5, 2018 •

edited by henrik-me

Loading

bgavrilMS Oct 5, 2018 •

edited by henrik-me

Loading

SomkaPe Oct 5, 2018 •

edited by henrik-me

Loading

henrik-me Oct 8, 2018 •

edited

Loading

SomkaPe Oct 8, 2018 •

edited by henrik-me

Loading

bgavrilMS Oct 10, 2018 •

edited by henrik-me

Loading

SomkaPe Oct 10, 2018 •

edited by henrik-me

Loading

SomkaPe Oct 19, 2018 •

edited by henrik-me

Loading

bgavrilMS Oct 5, 2018 •

edited by henrik-me

Loading

bgavrilMS Oct 5, 2018 •

edited by henrik-me

Loading

SomkaPe Oct 5, 2018 •

edited by henrik-me

Loading

bgavrilMS Oct 8, 2018 •

edited by henrik-me

Loading

bgavrilMS Oct 5, 2018 •

edited by henrik-me

Loading

SomkaPe Oct 5, 2018 •

edited by henrik-me

Loading

bgavrilMS Oct 5, 2018 •

edited by henrik-me

Loading

SomkaPe Oct 5, 2018 •

edited by henrik-me

Loading

bgavrilMS Oct 5, 2018 •

edited by henrik-me

Loading

SomkaPe Oct 5, 2018 •

edited by henrik-me

Loading

MarkZuber Oct 5, 2018 •

edited by henrik-me

Loading

SomkaPe Oct 5, 2018 •

edited by henrik-me

Loading

bgavrilMS Oct 8, 2018 •

edited by henrik-me

Loading

henrik-me Oct 8, 2018 •

edited

Loading

SomkaPe Oct 8, 2018 •

edited by henrik-me

Loading

SomkaPe Oct 19, 2018 •

edited by henrik-me

Loading

henrik-me Oct 8, 2018 •

edited

Loading

SomkaPe Oct 8, 2018 •

edited by henrik-me

Loading

henrik-me Oct 8, 2018 •

edited

Loading

SomkaPe Oct 8, 2018 •

edited by henrik-me

Loading

henrik-me Oct 8, 2018 •

edited

Loading

SomkaPe Oct 8, 2018 •

edited by henrik-me

Loading

bgavrilMS Oct 8, 2018 •

edited by henrik-me

Loading

SomkaPe Oct 8, 2018 •

edited by henrik-me

Loading

henrik-me Oct 8, 2018 •

edited

Loading

SomkaPe Oct 8, 2018 •

edited by henrik-me

Loading

henrik-me Oct 8, 2018 •

edited

Loading

SomkaPe Oct 19, 2018 •

edited by henrik-me

Loading

henrik-me Oct 8, 2018 •

edited

Loading

henrik-me Oct 8, 2018 •

edited

Loading

henrik-me Oct 8, 2018 •

edited

Loading

SomkaPe Oct 8, 2018 •

edited by henrik-me

Loading

henrik-me Oct 8, 2018 •

edited

Loading

SomkaPe Oct 8, 2018 •

edited by henrik-me

Loading

henrik-me Oct 8, 2018 •

edited

Loading

bgavrilMS Oct 8, 2018 •

edited by henrik-me

Loading