-
Notifications
You must be signed in to change notification settings - Fork 696
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new test cases for Transitive Package into NuGet.Client-VSDaily Tests pipeline #6148
base: dev
Are you sure you want to change the base?
Changes from 4 commits
ccb5b6e
765867b
e0fea3b
c888b50
4df0268
39c2427
0fa446f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -6,6 +6,7 @@ | |||
using System.Diagnostics; | ||||
using System.Linq; | ||||
using FluentAssertions; | ||||
using Microsoft.TeamFoundation.Common; | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. I believe this is a LINQ extension method. Could you try replacing it with By the way, this happens to me often as well. IntelliSense chooses the TeamFoundation namespace for some reason, but I think using Linq directly is better. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The namespace There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just write your method. Can that value ever be null even? An empty check should be enough. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than write a method, we often use FluentAssertions: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks to everyone's suggestions, we have removed the namwspace and updated the assertion method with |
||||
using Microsoft.Test.Apex.Services; | ||||
using NuGet.PackageManagement.UI; | ||||
using NuGet.PackageManagement.UI.TestContract; | ||||
|
@@ -76,6 +77,11 @@ public void AssertPackageNameAndType(string packageId, PackageLevel packageLevel | |||
package.Id.Should().Be(packageId); | ||||
} | ||||
|
||||
public bool AssertPackageListIsNullOrEmpty() | ||||
{ | ||||
return _uiproject.PackageItems.IsNullOrEmpty(); | ||||
} | ||||
|
||||
public bool InstallPackageFromUI(string packageId, string version) | ||||
{ | ||||
Stopwatch sw = Stopwatch.StartNew(); | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not clear on why this assertion is made both before the
SearchPackageFromUI
call and after the call. Is there a common bug where installing shows the TopLevel package but a following Search does not show that TopLevel package?Should
uiwindow.AssertPackageNameAndType(transitivePackageName, NuGet.VisualStudio.PackageLevel.Transitive);
also be asserted after the Act?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your review. 😊
First question: We call these assertions before
SearchPackageFromUI
to make sure that the package is installed successfully with the top-level package under the “top-level packages” node and the transitive packages under the “transitive packages” node.The second question: the test case is search for packages under the “top-level packages” node, so after searching for top-level packages, only the top-level packages are displayed. So, we didn't add assert transitive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. I believe also since the search is for an exact match package ID, there should be no transitive shown.
It may be worth considering a test where you search "Contoso" and find both a top-level
Contoso.A
and a transitiveContoso.C
, or similar.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your suggestion, we have added this test case
SearchTopLevelAndTransitivePackagePublicFieldInInstalledTabFromUI
to this PR, please help to review it, thanks again.