Permissions tests for API Key list and create actions #5153
Conversation
scottbommarito
requested review from
joelverhagen,
agr,
loic-sharma,
xavierdecoster,
shishirx34,
nugetlurker,
ryuyu,
chenriksson,
cristinamanum and
skofman1
|
|
||
| var model = GetModelForApiKeys(currentUser); | ||
|
|
||
| Assert.True(model.PackageOwners.Any(o => o.Owner == currentUser.Username && o.CanPushNew)); |
There was a problem hiding this comment.
Shouldn't this test that PackageOwners contains a single entry for the current user? Maybe test name could be something like PackageOwners_ContainsSingleCurrentUserEntryWithPushNew?
There was a problem hiding this comment.
Yes, I'll combine with the others
|
|
||
| var model = GetModelForApiKeys(currentUser); | ||
|
|
||
| Assert.True(model.PackageOwners.All(o => o.Owner == currentUser.Username && o.CanPushNew)); |
There was a problem hiding this comment.
Isn't this testing same as previous, but for site admin? Is All used instead of Any only because the FakeAdminUser isn't in an organization?
| var model = GetModelForApiKeys(currentUser); | ||
|
|
||
| Assert.True(model.PackageOwners.Any(o => o.Owner == currentUser.Username && o.CanPushNew)); | ||
| Assert.True(model.PackageOwners.Any(o => o.Owner == organization.Username && o.CanPushNew == isAdmin)); |
There was a problem hiding this comment.
The first assert is the same as the first test... current user should be the first entry. I could combine w/ that.
For the second assert, I would change it to test that PackageOwners contains a single entry for the Org account at index 2. Makes it a stronger test, and we'll catch if we have a bug with duplicate entries or not listing the current user first.
| var user = fakes.Admin; | ||
| var otherUser = fakes.User; | ||
|
|
||
| return WhenScopeOwnerDoesNotMatch_ReturnsBadRequest(user, otherUser); |
There was a problem hiding this comment.
fakes.Admin is site admin?
Can this be combined w/ previous test, and just take 2 Users as Theory args?
|
@chenriksson Fixed the tests as requested. |
|
|
||
| Assert.Equal(1, model.PackageOwners.Count(o => o.Owner == organization.Username && o.CanPushNew == isAdmin)); | ||
| } | ||
| public static IEnumerable<object[]> OrganizationIsNotInPackageOwnersIfNotMember_Data |
| { | ||
| get | ||
| { | ||
| foreach (var currentUser in |
There was a problem hiding this comment.
Can this just be:
yield return new object[] { TestUtility.FakeUser };
yield return new object[] { TestUtility.FakeAdminUser };
There was a problem hiding this comment.
If we have a lot of test code converting entities to object arrays, could consider a MemberDataHelper class:
public static class MemberDataHelper {
object[] AsMemberData(this User user) { return new object[] { user }; }
}
public class TestClass {
public static IEnumerable<object[]> TestData {
get {
yield return TestUtility.FakeUser.AsMemberData();
yield return TestUtility.FakeAdminUser.AsMemberData();
}
}
[Theory]
[MemberData(nameof(TestData))]
public void TestMethod(User user) { ... }
}
There was a problem hiding this comment.
How about
public static class MemberDataHelper {
public static object[] AsData(params object[] objects)
{
return objects;
}
}
public class TestClass {
public static IEnumerable<object[]> TestData {
get {
yield return MemberDataHelper.AsData(TestUtility.FakeUser);
yield return MemberDataHelper.AsData(TestUtility.FakeAdminUser);
}
}
[Theory]
[MemberData(nameof(TestData))]
public void TestMethod(User user) { ... }
}
|
|
||
| var model = GetModelForApiKeys(currentUser); | ||
|
|
||
| Assert.True(!model.PackageOwners.Any(o => o.Owner == organization.Username)); |
There was a problem hiding this comment.
Asserting count instead of Any would provide a better message.
.
No description provided.