-
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?
Conversation
test/NuGet.Tests.Apex/NuGet.Tests.Apex/Apex/NuGetUIProjectTestExtension.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Jeff Kluge <[email protected]>
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
using Microsoft.TeamFoundation.Common; |
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.
The IsNullOrEmpty()
method need this using Microsoft.TeamFoundation.Common;
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.
Ok. I believe this is a LINQ extension method. Could you try replacing it with using System.Linq;
?
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 comment
The reason will be displayed to describe this comment to others. Learn more.
The namespace "using System.Linq;"
has been reference in this file #line 7, but the using Microsoft.TeamFoundation.Common;
still been prompted for a reference when using the IsNullOrEmpty()
method.
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.
Just write your method.
Let's not take dependencies in "generic" apis from packages not meant to deliver generic code.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than write a method, we often use FluentAssertions: collection.Should().BeNullOrEmpty();
See https://fluentassertions.com/collections/
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 to everyone's suggestions, we have removed the namwspace and updated the assertion method with collection.Should().BeNullOrEmpty();
, could you please help to review it, thanks.
test/NuGet.Tests.Apex/NuGet.Tests.Apex/Apex/NuGetUIProjectTestExtension.cs
Outdated
Show resolved
Hide resolved
uiwindow.AssertPackageNameAndType(TestPackageName, NuGet.VisualStudio.PackageLevel.TopLevel); | ||
uiwindow.AssertPackageNameAndType(transitivePackageName, NuGet.VisualStudio.PackageLevel.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.
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 transitive Contoso.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.
Bug
Fixes: https://github.com/NuGet/Client.Engineering/issues/3070
Description
Add the below three new test cases about the "Transitive Package" feature to the NuGet.Client-VSDaily Tests pipeline.
Search a top-level package in "Installed" tab
Install a top level package which have a transitive package
Verify uninstalling a top-level package will uninstall the transitive package automatically
PR Checklist