Updating via Checkboxes no longer causes Null Reference Exception #3864
Conversation
src/NuGet.Clients/NuGet.PackageManagement.UI/Xamls/PackageManagerControl.xaml.cs
Show resolved
Hide resolved
| } | ||
|
|
||
| /// <summary> | ||
| /// Repro for a bug caused by a NullReferenceException being thrown due to a null <see cref="PackageIdentity.Version"/> |
There was a problem hiding this comment.
What kind of value does this test provide us beyond being great for this review?
It's an input error, not necessarily a contract we're going to try to maintain.
There was a problem hiding this comment.
The input error is "by design". So you either handle the input error (I did not change this),
or you have a test proving it does what you found it does. It also helps someone see this code depends on non-null versions
Then, you have a test that can be easily flipped if/when we decide on a different design. Most of the test work is done, and the PR will be more clear.
There was a problem hiding this comment.
Why did you decide against adding an input check?
Will the call always fail if the input PackageIdentity has a null version?
There was a problem hiding this comment.
Because I'm trying to fix a bug specifically. Not redesign the inners of Package Manager.
Not always, nope. Has to be BuildIntegrated project, with at least 1 update available.
There was a problem hiding this comment.
Because I'm trying to fix a bug specifically. Not redesign the inners of Package Manager.
I think this can be considered in scope.
Changing the NuGetPackageManager class to ensure we don't run into unexpected exceptions is in scope.
Especially because there wouldn't be any side-effects.
There was a problem hiding this comment.
Especially because there wouldn't be any side-effects.
I don't have proof of that. And it's a 16.9 blocking bug, so not the time to start paying our debts. I think as part of 16.10 refactoring, this could be evaluated.
There was a problem hiding this comment.
Synced up offline.
Call it good for 16.9, consider additional quality improvements in NuGetPackageManager for 16.10.
Bug
Fixes: NuGet/Home#9882
Regression? Last working version: 16.7
Description
Use an always populated Version object.
LatestVersionis only set on a subset of packages.Versionis always set.A new unit test demonstrates the logic that will throw for a null version.
Added an
ArgumentNullExceptionthat could cause a similar error.PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation