Do not warn http sources in search/list scenarios when allowInsecureConnections is set to true. #5371
Conversation
| @@ -1169,6 +1170,65 @@ public void ListCommand_WhenListWithHttpSource_Warns() | |||
| Assert.Contains("WARNING: You are running the 'list' operation with an 'HTTP' source", result.AllOutput); | |||
| } | |||
|
|
|||
| [PlatformTheory(Platform.Windows)] | |||
There was a problem hiding this comment.
Similar feedback as the other PR: #5363.
Checking for false is really just checking the parsing in the settings. We probably don't need those.
Both tests have the same concern.
Rest looks good.
There was a problem hiding this comment.
Thanks @nkolev92 for reviewing!
I think having both true and false in the same test looks more clear, as there is a contrast and it proves the flag works correctly.
To avoid testing duplicate scenarios, how about removing the ListCommand_WhenListWithHttpSource_Warns? As it tests null AllowInsecureConnections, which is the same with false.
There was a problem hiding this comment.
I removed three tests which tests the null AllowInsecureConnections and kept the three false AllowInsecureConnections tests.
| Util.CreateFile(packageDirectory, "nuget.config", $@" | ||
| <configuration> | ||
| <packageSources> | ||
| <add key='http-feed' value='{server.Uri}nuget' allowInsecureConnections=""{allowInsecureConnections}"" /> |
There was a problem hiding this comment.
Question: Do we have any tests where we validate that the absence of this attributes results in warnings for http connections?
There was a problem hiding this comment.
Thanks for reviewing @aortiz-msft !
There used to be a ListCommand_WhenListWithHttpSource_Warns test, testing the absence of this attributes results in http warning.
But I just removed it in this PR, to address this comment .
Since the default value of allowInsecureConnections is false, absence of this attributes is the same with setting it to false.
There was a problem hiding this comment.
In other words, yes we have a test that validates the absence of the attribute results in the default value:
| @@ -1099,7 +1106,21 @@ public void SearchCommand_WhenSearchWithHttpSources_Warns() | |||
| Assert.True(result.Success, $"{result.AllOutput}"); | |||
| Assert.Contains("No results found.", $"{result.AllOutput}"); | |||
| Assert.DoesNotContain(">", $"{result.AllOutput}"); | |||
| Assert.Contains("WARNING: You are running the 'search' operation with an 'HTTP' source", result.AllOutput); | |||
|
|
|||
| string acutalOutputWithoutSpace = SettingsTestUtils.RemoveWhitespace(result.Output); | |||
There was a problem hiding this comment.
typo: acutal should be actual
There was a problem hiding this comment.
Sorry! Fixed the typo just now.
…onnections is set to true. (#5371)
…onnections is set to true. (#5371)
…onnections is set to true. (#5371)
Bug
Fixes: NuGet/Home#12790
Regression? Last working version:
Description
When
allowInsecureConnectionsproperty inpackageSourcessection is set to true in NuGet.Config files, do not warn for HTTP sources in search/list scenarios.The PRs enable the HTTP warnings in those scenarios are:
#4629
#4628
PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation