-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Add baseline version test #7627
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
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.
var localAssemblyVersion = AssemblyName.GetAssemblyName(file)?.Version; | ||
var splitPath = file.Split('\\'); | ||
var dllName = splitPath[splitPath.Length - 1]; | ||
Assert.True(nugetAssemblyVersions.ContainsKey(dllName), $"Expected {dllName} to be in the downloaded dlls"); |
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.
Oh and BTW just checking, are there are no xUnit asserts for these two asserts here? Usually Assert.True
is frowned upon, unless there's no other reasonable choice.
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.
Assert.Contains(nugetAssemblyVersions.Keys, ...)
should work here. The only supposed loss would be the custom error message when it fails.
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 could change to use the Assert.Contains
api. The error message becomes slightly less clear. I'm indifferent.
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. Please send TELL MODE email to shiproom alias with FYI (specify it's for 2.1). I sent you an email with info.
@@ -34,6 +38,7 @@ | |||
<PackageReference Include="xunit" Version="$(XunitPackageVersion)" /> | |||
<PackageReference Include="xunit.analyzers" Version="$(XunitAnalyzersPackageVersion)" /> | |||
<PackageReference Include="xunit.runner.visualstudio" Version="$(XunitRunnerVisualstudioPackageVersion)" /> | |||
<PackageReference Include="McMaster.Extensions.CommandLineUtils" Version="2.2.2" /> |
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 know this is test-only but have we used @natemcmaster's private packages elsewhere?
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 mimicked here what we did in the internal repo.
var localAssemblyVersion = AssemblyName.GetAssemblyName(file)?.Version; | ||
var splitPath = file.Split('\\'); | ||
var dllName = splitPath[splitPath.Length - 1]; | ||
Assert.True(nugetAssemblyVersions.ContainsKey(dllName), $"Expected {dllName} to be in the downloaded dlls"); |
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.
Assert.Contains(nugetAssemblyVersions.Keys, ...)
should work here. The only supposed loss would be the custom error message when it fails.
Issue: #7546