Fix null reference errors in tests #23091
Conversation
|
I'm half tempted to turn off nullability warnings from tests. They are useful to help "figure out" the implementation at times, but not particularly high value. We could consider that if this is a recurrent problem. |
Pilchie
added
area-healthchecks
dougbu
left a comment
dougbu
left a comment
There was a problem hiding this comment.
Eight warnings from tasks outside msbuild. Wahoo 🥇
| var builder = new ApplicationBuilder(serviceProvider: null); | ||
| var noMiddleware = new ApplicationBuilder(serviceProvider: null).Build(); | ||
| var builder = new ApplicationBuilder(serviceProvider: null!); | ||
| var noMiddleware = new ApplicationBuilder(serviceProvider: null!).Build(); |
There was a problem hiding this comment.
Suggestion only: Seem to be working around the nullability of this constructor parameter a fair amount. Might be easier to either provide a default constructor or allow null in this constructor for testing purposes. Either way, shouldn't need to change the type of the ApplicationServices property (though it's odd the ApplicationBuilder properties aren't nullable given GetProperty(...) can return null).
There was a problem hiding this comment.
There was a problem hiding this comment.
I guess I can do that in a follow up. Let's get this PR in
There was a problem hiding this comment.
The API is very inconsistent.
- Adding a parameterless constructor would simplify usage in tests.
- ApplicationServices_Get should return a static no-op instance rather than null.
- ServerFeatures should also return a read-only no-op instance.
There was a problem hiding this comment.
Captured this in a separate feedback. #23232.
amcasey
added
area-networking
Fixes #22992