Permissions tests for Edit, Delete, Manage Package Owners, and SetLicenseReportVisibility (UI) #5151
Conversation
scottbommarito
requested review from
joelverhagen,
agr,
loic-sharma,
xavierdecoster,
shishirx34,
nugetlurker,
ryuyu,
chenriksson,
cristinamanum and
skofman1
scottbommarito
changed the title
scottbommarito
changed the title
| { | ||
| get | ||
| { | ||
| foreach (var ownerData in Owner_Data) |
There was a problem hiding this comment.
Might look better in test explorer if rewritten as LINQ expression body.
There was a problem hiding this comment.
(well, probably all of those would have to be rewritten as expression bodied properties, probably not worth it)
There was a problem hiding this comment.
I didn't think it made much of a difference--I thought it just serialized all of the properties of the object regardless of how it was enumerated.
Either case, I'm going to leave this because as you said, it would have to become expression bodied properties.
| var result = controller.Delete(_packageRegistration.Id, _package.Version); | ||
|
|
||
| Assert.IsType<HttpNotFoundResult>(result); | ||
| } |
| public void DisplaysFullVersionStringAndUsesNormalizedVersionsInUrlsInSelectList() | ||
| private static string _packageId = "CrestedGecko"; | ||
| private static PackageRegistration _packageRegistration; | ||
| private static Package _package; |
There was a problem hiding this comment.
I would beware of reusing the same instance of these test objects across tests... in case changes in state could potentially lead to intermittent test failures.
There was a problem hiding this comment.
I know some of the other tests had this, but they might not have been static. Will check.
There was a problem hiding this comment.
removed static
There was a problem hiding this comment.
Thanks - assuming new instance of test class is created for each test case? i.e., class variables won't be shared?
There was a problem hiding this comment.
I've seen some tests with this existing pattern (ReportMyPackage for example). Also, if there was any sort of concurrency issue here, I would expect my tests to have failed when I tested them.
| Reason = "Other", | ||
| Signature = "John Doe" | ||
| }; | ||
| yield return new object[] |
There was a problem hiding this comment.
This has a similar pattern to other tests you added that take (currentUser, owner). Did we add a helper in that case to simplify the code?
yield return MemberDataHelpers.UserOnBehalfOfAccount(TestUtility.FakeUser, new User { Key = 5535 });
There was a problem hiding this comment.
Helper was added, but because this is a feature branch and I don't want to deal with any annoying rebasing, I was planning on integrating it with the tests that don't have it in a later PR.
| private static string _packageId = "CrestedGecko"; | ||
| private static string _packageVersion = "3.4.2"; | ||
|
|
||
| private static Package _package = new Package |
There was a problem hiding this comment.
Beware of reusing this instance across tests. Consider if the test cases run in parallel, or depend on state which changes.
|
@chenriksson I fixed your comments! |
|
|
||
| [Theory] | ||
| [MemberData(nameof(NotOwner_Data))] | ||
| public async Task Returns401IfNotOwner(User currentUser, User owner, bool visible) |
There was a problem hiding this comment.
should be passed into SetLicenseReportVisibility--updated to do this
| var result = await controller.SetLicenseReportVisibility("Foo", "1.0", visible: false, urlFactory: (pkg, relativeUrl) => @"~\Bar.cshtml"); | ||
|
|
||
| // Assert | ||
| Assert.IsType<HttpStatusCodeResult>(result); |
There was a problem hiding this comment.
When there is a 401 assertion, we should also verify the dependency (e.g. DB) is not being changed. Applies to all tests here.
There was a problem hiding this comment.
Upon further investigation, in this particular test, it is already verified because the package service is a strict mock. I do need to analyze the rest of the tests however.
.…enseReportVisibility (UI) (#5151)
…enseReportVisibility (UI) (#5151)
…enseReportVisibility (UI) (#5151)
…enseReportVisibility (UI) (#5151)
…enseReportVisibility (UI) (#5151)
…enseReportVisibility (UI) (#5151)
For Edit, SetLicenseReportVisibility, and Delete I parametrized the tests so that they now run with every test user.
For Manage Package Owners I added two simple tests to guarantee it displays the form when expected.