Moved POCOs to EF and fixed tests #378

suhasj · 2015-02-27T20:23:12Z

@HaoK I moved the POCOs to Identity.EF. The Identity.Test and Identity.InMemory.Test broke due to this change since they did not reference Identity.EF. Now we could either duplicate the code in the tests or reference the assembly and include the namespace. In addition the UserManagerTestBase is shared so that has to updated also. I choose to reference the assembly as this seems more cleaner. I made sure we reference only the POCOs and nothing else. Let me know if there is a better way of doing this.

/cc @divega

You can skip the nitpick feedback for this PR

HaoK · 2015-02-27T22:24:02Z

Nah, its a bad idea to add EF dependencies, just copy the pocos into the shared test code and rename them for the tests

suhasj · 2015-02-27T22:44:55Z

In doing so, I have replicate the code in UserManagerTestBase for InMemory.Test project since it uses the IdentityUser object and that is a lot of duplication. Is that the right way ?

HaoK · 2015-02-27T22:53:33Z

There should be no replication other than copying the pocos from Core to the Shared Test code along side user manager test base with a new name for the pocos

suhasj · 2015-02-27T23:11:26Z

Ok let me see if the amount of duplicated code will be difficult to maintain in the future. I can reason out accordingly after that

suhasj · 2015-02-27T23:58:05Z

Now if I duplicate the POCOs in the Shared folder and make UserManagerTestBase use them, the SqlStoreBase complains since it uses UserManagerTestBase with IdentityUser from Identity.EF. If I make all the test projects to use the POCOs in the shared folder, then we need to maintain them along with the POCOs in the source. My concern is that if we forget to update them then the tests will pass incorrectly.

HaoK · 2015-02-28T00:01:06Z

Or maybe just move the IdentityUser classes to the new shared code stuff, so both EF and tests can depend on it safely.

HaoK · 2015-02-28T00:03:44Z

Another route could be to look at what the tests are doing to access the pocos directly and instead replace them with calls to identity, which might let you remove the IdentityUser constraint entirely

suhasj · 2015-03-02T17:41:08Z

Sure the second option might be an easy option. Let me try that out

suhasj · 2015-03-03T21:16:52Z

@HaoK @divega

Updated with changes

UserManagerTestBase now is agnostic to user model and gets all attributes using manager apis.
Identity POCOs in Shared folder to be used by InMemory and Core tests
updated samples

Question

New extension method on ServiceCollection AddIdentityWithTestPocos which is used only in test because it uses the Shared Pocos while the one is source uses EF ones. Instead of this should i move the IdentityBuilderTests and IdentityOptionsTests to EF ? I would want to go with move option

divega · 2015-03-03T21:22:50Z

src/Microsoft.AspNet.Identity.EntityFramework/IdentityServiceCollectionExtensions.cs

+{
+    public static class IdentityServiceCollectionExtensions
+    {
+        public static IdentityBuilder AddIdentity(this IServiceCollection services)


Interesting that these overload now moves here because the typed moved and hence they are tied to EF. I think it would probably be better to get rid of them.

Fixed and updated

suhasj · 2015-03-03T22:56:47Z

Also one important feedback i need to getting username synchronously inside a Linq queries. For example
mgr.GetUserNameAsync(u).Result in https://github.com/aspnet/Identity/pull/378/files#diff-bc862db146cf4a686c23d4fb0676c99eR584. The reason for doing this is that the Linq query cannot be async. Let me know if this is something that is not recommended.

The workaround is to have a sync method on the UserManagerTestBase class which returns different user attributes. For example GetUserId(user). This is abstract on the base and is implemented by the concrete test classes

divega · 2015-03-04T00:14:30Z

test/Shared/IdentityUser.cs

+        /// <summary>
+        ///     Navigation property for users in the role
+        /// </summary>
+        public virtual ICollection<IdentityUserRole<TKey>> Roles { get; private set; } = new List<IdentityUserRole<TKey>>();


Will private create issues with reflection on Mono?

Why do we need navigation properties on the shared test pocos?

True but I wanted to have parity with the Pocos in the source. The navigation property can be deleted in test pocos

@divega for the private thing, the issue is fixed in the latest version of Mono. But should we revert it to protected ? Do we foresee users overriding this property considering it is a navigation property ?

Its not about parity, the EF pocos are EF specific, if the navigation properties are not needed for the tests, they shouldn't be there.

so the navigation properties are needed since the InMemory tests makes use of them. I need to keep them

divega · 2015-03-04T00:31:12Z

mgr.GetUserNameAsync(u).Result in https://github.com/aspnet/Identity/pull/378/files#diff-bc862db146cf4a686c23d4fb0676c99eR584. The reason for doing this is that the Linq query cannot be async. Let me know if this is something that is not recommended.

Can't you do this?

var userName = await mgr.GetUserNameAsync(u);
Assert.Null(mgr.Users.FirstOrDefault(u => userName == "bogus"));

divega · 2015-03-04T00:36:28Z

New extension method on ServiceCollection AddIdentityWithTestPocos which is used only in test because it uses the Shared Pocos while the one is source uses EF ones. Instead of this should i move the IdentityBuilderTests and IdentityOptionsTests to EF ? I would want to go with move option

I don't understand this question, and I couldn't find the method right now. Did you already remove it? Were these test be independent of the provider?

HaoK · 2015-03-04T00:59:50Z

test/Microsoft.AspNet.Identity.EntityFramework.Test/DefaultPocoTest.cs

@@ -65,6 +67,152 @@ public void DropDatabaseStart()
            IdentityResultAssert.IsSuccess(await userManager.DeleteAsync(user));
        }

+        [Fact]


Are these new tests or copied tests?

These are new ones I added to test if navigation properties work when used with Include on dbContext

suhasj · 2015-03-04T02:41:39Z

@divega Using a variable ! so obvious. To answer the second question, yes the class no longer exists so it is fine

suhasj · 2015-03-04T18:10:48Z

@divega Now I recall that I did not use the variable since i need to use the object inside the Where clause. So var userName = await mgr.GetUserNameAsync(u); would not work since u is to be got from the lambda. The workaround I see is to use a foreach loop instead of Where which is not clean either.

divega · 2015-03-04T18:41:19Z

Ah, I missed that. Sorry.

suhasj · 2015-03-05T01:47:59Z

Updated with feedback

Removed InMemoryUser and ApplicationUser. Identity.Test and IdentityInMemory.Test refer to TestUser only
GetPasswordHash an abstract method on UserManagerTestBase
CreateTestUser now takes namePrefix instead of username
CreateTestRole does not take a namePrefix since most test need a expected rolename for further assertions.

suhasj · 2015-03-12T21:21:44Z

@HaoK @divega

Removing the feedback tag. I have addressed all feedback so wanted to make sure it is ready to check in

divega · 2015-03-12T22:47:25Z

I'll take another look later today.

HaoK · 2015-03-13T21:41:05Z

You might want to rebase since I added some functionals in InMemory yesterday, but otherwise looks to me

suhasj · 2015-03-18T04:44:28Z

@divega pinging a reminder

divega · 2015-03-20T09:01:19Z

test/Microsoft.AspNet.Identity.EntityFramework.Test/DefaultPocoTest.cs

+            });
+
+            var userManager = builder.ApplicationServices.GetRequiredService<UserManager<IdentityUser>>();
+            var dbContext = builder.ApplicationServices.GetRequiredService<IdentityDbContext>();


~~DbContext is IDisposable. Please use using even if doesn't seem needed right now.~~

Sorry, scratch that. I see now that you don't control the lifetime of this one directly.

However this still doesn't seem right. When will the context get disposed? I assume you copied this pattern from other tests?

I am assuming you are worried how the database is cleared ? There is a test DropDatabaseStart that is marked with lowest priority that will delete the database after all the tests in the class have run. This class existed all the way long. I have only added more tests that's all. Regarding the object, since it is created in the scope of the test, then it should be disposed at the end of it. If adding a using statement for DBContext is the recommended practice then sure I can update that but will have to do it in other tests as well

I am more worried about the risk of tests leaking resources, e.g. connections, or even memory depending how (well) the DI container implementation works and what we do in the tests.

suhasj · 2015-03-23T04:55:21Z

Rebased with latest and remove all instances of .Result in the UserManagerTestBase. Added the predicate to compare user/roles names. Needed to add a contains flag since has two asserts that needed to test part of user/roles names.

Also noop for IQueryable tests is the managers do not support them

divega · 2015-03-23T20:15:59Z

test/Shared/UserManagerTestBase.cs

+
+        protected abstract TRole CreateTestRole(string roleName = "");
+
+        protected abstract Expression<Func<TUser, bool>> UserNamePredicate(string userName, bool contains = false);


I would rather simplify the tests to just succeed with simple equality and get rid of the contains flag on both predicates.

Contains is not the same as StartsWith.

Other than that, I think we are very close.

Yeah Contains != StartsWith, should be StartsWith

We can't simplify the tests to just do equality since we aren't cleaning up the database, so a lot of the tests use the same username prefix, like "test", and we only drop/recreate the database once per run. Rather than trying to manually maintain user name uniqueness, I went with the approach of randomizing the names instead by always jamming on a guid, and then doing StratsWith instead of strict equality.

We were doing name + guid so I presumed Contains would still be valid check instead of StartWith considering we are looking if part of the string is present. I'll revert back to StartsWith it is just that I have to now maintain the original randomized username and pass it exactly to the predicate

I am ok with having StartsWith checks if we can't have just do equality. But let's then split this in 4 simple predicate methods without any flags: GetUserNameEqualsPredicate(string), GetUserNameStartsWithPredicate(string), GetRoleNameEqualsPredicate(string), GetRoleNameStartsWithPredicate(string).

Added the four predicates and restored the test to original assert statements

suhasj · 2015-03-23T23:37:39Z

Updated with 2 additional predicates to compare StartsWith for user/role name

Some changes you might be interested in predicates
queryable tests 1 and 2

@ajcvickers made changes to EF for mapping. Should I rebase and send another PR to see if that works with these changes ?

suhasj · 2015-03-24T00:47:11Z

I rebased it to latest EF which had renaming for mapping in OnModel creating. I have updated the IdentityDbContext with the latest changes

@ajcvickers can you please specifically review changes to IdentityDbContext

divega · 2015-03-24T05:26:05Z

test/Shared/UserManagerTestBase.cs

+                }
+                Assert.Equal(roles.Count, manager.Roles.Count(RoleNameStartsWithPredicate("CanQuerableRolesTest")));
+                var r1 = manager.Roles.FirstOrDefault(RoleNameStartsWithPredicate("CanQuerableRolesTest1"));
+                Assert.Equal(roles[1], r1);


Can we have a negative assert as in the Users test and standardize on the same test data generation pattern?

sure I updated it and role name are done the same way as usernames.

ajcvickers · 2015-03-24T16:45:41Z

IdentityDbContext changes look fine.

divega · 2015-03-24T18:46:14Z

to get it on beta4. We can address any outstanding test issues later.

suhasj · 2015-03-24T18:47:08Z

thanks to everybody for patiently reviewing this

ghost added the cla-not-required label Feb 27, 2015

suhasj force-pushed the MovePOCO branch from ada908a to fbf32bd Compare March 3, 2015 21:11

divega reviewed Mar 3, 2015
View reviewed changes

divega reviewed Mar 4, 2015
View reviewed changes

HaoK reviewed Mar 4, 2015
View reviewed changes

suhasj force-pushed the MovePOCO branch from 5daaa5b to 04d454e Compare March 12, 2015 21:20

suhasj changed the title ~~[Feedback PR] Moved POCOs to EF and fixed tests~~ Moved POCOs to EF and fixed tests Mar 12, 2015

divega reviewed Mar 20, 2015
View reviewed changes

suhasj force-pushed the MovePOCO branch from f9c88da to 90cf9fe Compare March 23, 2015 04:38

divega reviewed Mar 23, 2015
View reviewed changes

Suhas Joshi added 2 commits March 23, 2015 17:06

Moved POCOs to EF and fixed tests

795facf

Renaming mapping due to latest EF changes

cd82560

suhasj force-pushed the MovePOCO branch from 7ccf95c to cd82560 Compare March 24, 2015 00:44

divega reviewed Mar 24, 2015
View reviewed changes

Feedback

739ba98

Removed setters for nav properties

df3a7e3

suhasj closed this Mar 24, 2015

suhasj deleted the MovePOCO branch March 24, 2015 20:30

suhasj mentioned this pull request Mar 24, 2015

Moving IdentityUser and other POCOs to core seems to lock this more to EF, excluding other databases #332

Closed


		protected abstract TRole CreateTestRole(string roleName = "");

		protected abstract Expression<Func<TUser, bool>> UserNamePredicate(string userName, bool contains = false);

Moved POCOs to EF and fixed tests #378

Moved POCOs to EF and fixed tests #378

Conversation

suhasj commented Feb 27, 2015

HaoK commented Feb 27, 2015

suhasj commented Feb 27, 2015

HaoK commented Feb 27, 2015

suhasj commented Feb 27, 2015

suhasj commented Feb 27, 2015

HaoK commented Feb 28, 2015

HaoK commented Feb 28, 2015

suhasj commented Mar 2, 2015

suhasj commented Mar 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suhasj commented Mar 3, 2015

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

Choose a reason for hiding this comment

divega commented Mar 4, 2015

divega commented Mar 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suhasj commented Mar 4, 2015

suhasj commented Mar 4, 2015

divega commented Mar 4, 2015

suhasj commented Mar 5, 2015

suhasj commented Mar 12, 2015

divega commented Mar 12, 2015

HaoK commented Mar 13, 2015

suhasj commented Mar 18, 2015

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

Choose a reason for hiding this comment

suhasj commented Mar 23, 2015

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suhasj commented Mar 23, 2015

suhasj commented Mar 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajcvickers commented Mar 24, 2015

divega commented Mar 24, 2015

suhasj commented Mar 24, 2015