-
Notifications
You must be signed in to change notification settings - Fork 686
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
Use SdkAnalysisLevel in restorecommand http-errors #5925
Use SdkAnalysisLevel in restorecommand http-errors #5925
Conversation
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.
Looks good.
Some minor feedback.
test/NuGet.Core.FuncTests/NuGet.Commands.FuncTest/RestoreCommandTests.cs
Outdated
Show resolved
Hide resolved
test/NuGet.Core.FuncTests/NuGet.Commands.FuncTest/RestoreCommandTests.cs
Outdated
Show resolved
Hide resolved
test/NuGet.Core.FuncTests/NuGet.Commands.FuncTest/RestoreCommandTests.cs
Outdated
Show resolved
Hide resolved
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 feel somewhat strongly about not using potentially valid DNS host names in tests. Otherwise I'd be happy to approve this.
I know there's too much in the coding guidelines document to remember everything, but is it worthwhile adding something in there, or create a new testing-guidelines.md
, so that in the future we can just link to it (plus maybe someone will actually read it and remember when writing a test, leading to fewer PR iterations)
/// </summary> | ||
internal static class SdkAnalysisLevelMinimums | ||
{ | ||
public const string HttpErrorSdkAnalysisLevelMinimumValue = "9.0.100"; |
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.
Thinking ahead to a future where we have multiple features that are gating on the same SdkAnalysisLevel version, would it be better to have one SdkAnalysisLevelMinimums.v9_0_100
that they all use, or multiple feature-named properties with the same value?
The compiler will de-duplicate compile time constant strings, so that's not a concern.
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 prefer having feature-based constants. It gives context to people reading the code.
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 code where it's reference is context too 😁
test/NuGet.Core.FuncTests/NuGet.Commands.FuncTest/RestoreCommandTests.cs
Outdated
Show resolved
Hide resolved
string httpSourceUrl = "http://api.source/index.json"; | ||
string httpsSourceUrl = "https://api.source/index.json"; |
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 don't think the https feed plays any part in this test, right? If that's right, it makes the test longer, and takes me longer to read/verify the test because now I'm trying to figure out why this second source is relevant to the test.
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.
It's just to make sure we are throwing errors despite having both http and https sources.
test/NuGet.Core.FuncTests/NuGet.Commands.FuncTest/RestoreCommandTests.cs
Outdated
Show resolved
Hide resolved
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 like the separate API.
Maybe we keep it internal in NuGet.Commands until we have another use-case.
3 use-cases tends to be the sweet spot for designing a good API, and currently we just have 1. An idea would be something that combines feature flags alongside the SDK analysis level, but really all of this would be easier if we had more use-cases.
src/NuGet.Core/NuGet.Versioning/utility/SdkAnalysisLevelMinimums.cs
Outdated
Show resolved
Hide resolved
[Fact] | ||
public async Task Restore_WithHttpSourceAndAllowInsecureConnectionsTrue_Succeeds() | ||
{ | ||
// Arrange | ||
using var pathContext = new SimpleTestPathContext(); | ||
var packageA = new SimpleTestPackageContext("a", "1.0.0"); | ||
await SimpleTestPackageUtility.CreateFolderFeedV3Async(pathContext.PackageSource, packageA); | ||
string httpSourceUrl = "http://api.source/index.json"; | ||
string httpsSourceUrl = "https://api.source/index.json"; | ||
string httpSourceUrl = "http://unit.test/index.json"; |
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 fyi, you might see a lot of tests referring to contoso.
Here's some history: https://learn.microsoft.com/en-us/microsoft-365/enterprise/contoso-case-study?view=o365-worldwide.
src/NuGet.Core/NuGet.Versioning/utility/SdkAnalysisLevelMinimums.cs
Outdated
Show resolved
Hide resolved
src/NuGet.Core/NuGet.Versioning/utility/SdkAnalysisLevelMinimums.cs
Outdated
Show resolved
Hide resolved
[Fact] | ||
public void IsEnabled_WhenSdkAnalysisLevelIsNullAndUsingMicrosoftNetSdkIsFalse_ShouldReturnTrue() | ||
{ | ||
var result = SdkAnalysisLevelMinimums.IsEnabled(null, false, new NuGetVersion("9.0.100")); | ||
Assert.True(result); | ||
} | ||
|
||
[Fact] | ||
public void IsEnabled_WhenSdkAnalysisLevelIsNullAndUsingMicrosoftNetSdkIsTrue_ShouldReturnFalse() | ||
{ | ||
var result = SdkAnalysisLevelMinimums.IsEnabled(null, true, new NuGetVersion("9.0.100")); | ||
Assert.False(result); | ||
} | ||
|
||
[Fact] | ||
public void IsEnabled_WhenSdkAnalysisLevelIsLessThanMinSdkVersion_ShouldReturnFalse() | ||
{ | ||
var result = SdkAnalysisLevelMinimums.IsEnabled(new NuGetVersion("8.0.900"), true, new NuGetVersion("9.0.100")); | ||
Assert.False(result); | ||
} | ||
|
||
[Fact] | ||
public void IsEnabled_WhenSdkAnalysisLevelIsEqualToMinSdkVersion_ShouldReturnTrue() | ||
{ | ||
var result = SdkAnalysisLevelMinimums.IsEnabled(new NuGetVersion("9.0.100"), true, new NuGetVersion("9.0.100")); | ||
Assert.True(result); | ||
} | ||
|
||
[Fact] | ||
public void IsEnabled_WhenSdkAnalysisLevelIsGreaterThanMinSdkVersion_ShouldReturnTrue() | ||
{ | ||
var result = SdkAnalysisLevelMinimums.IsEnabled(new NuGetVersion("9.0.101"), true, new NuGetVersion("9.0.100")); | ||
Assert.True(result); | ||
} |
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.
Nit: these could be combined into a data-driven test (using Theory) where you supply 4 parameters: the first three being the inputs, the fourth being the expectedValue (true or false).
await _logger.LogAsync(RestoreLogMessage.CreateError(NuGetLogCode.NU1302, | ||
var isErrorEnabled = SdkAnalysisLevelMinimums.IsEnabled(_request.Project.RestoreMetadata.SdkAnalysisLevel, | ||
_request.Project.RestoreMetadata.UsingMicrosoftNETSdk, | ||
SdkAnalysisLevelMinimums.HttpErrorSdkAnalysisLevelMinimumValue); |
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.
We can see here how "wordy" the property name is. The C# language doesn't let you (easily) use the property without also specifying the class name, and having "SdkAnalysisLevelMinimum" in both the class and property looks redundant when used.
But this is something easily changed later, since it's not a public API. Just something to keep in mind when writing new code.
Bug
Fixes: NuGet/Home#13546
Description
while performing restore, avoid showing error based on the SDKAnalysisLevel value.
if it is greater than or equal to 9.0.100 > show error message.
Concerns
Should we parse
SdkAnalysisLevel
andUsingMicrosoftNetSdk
for the following ?PR Checklist