Skip to content

Fix null reference errors in tests #23091

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 18, 2020
Merged

Fix null reference errors in tests #23091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Antiforgery/test/DefaultAntiforgeryTokenStoreTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ public async Task GetRequestTokens_NonFormContentType_UsesHeaderToken()
httpContext.Request.Headers.Add("header-name", "header-value");

// Will not be accessed
httpContext.Request.Form = null;
httpContext.Request.Form = null!;

var options = new AntiforgeryOptions
{
Expand All @@ -191,7 +191,7 @@ public async Task GetRequestTokens_NoHeaderToken_NonFormContentType_ReturnsNullT
httpContext.Request.ContentType = "application/json";

// Will not be accessed
httpContext.Request.Form = null;
httpContext.Request.Form = null!;

var options = new AntiforgeryOptions
{
Expand Down
2 changes: 1 addition & 1 deletion src/HealthChecks/HealthChecks/test/DependencyInjection/ServiceCollectionExtensionsTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ public void AddHealthChecks_RegistersPublisherService_WhenOtherHostedServicesReg
services.AddHealthChecks();

// Assert
Assert.Collection(services.OrderBy(s => s.ServiceType.FullName).ThenBy(s => s.ImplementationType.FullName),
Assert.Collection(services.OrderBy(s => s.ServiceType.FullName).ThenBy(s => s.ImplementationType!.FullName),
actual =>
{
Assert.Equal(ServiceLifetime.Singleton, actual.Lifetime);
Expand Down
18 changes: 9 additions & 9 deletions src/Http/Http.Abstractions/test/MapPathMiddlewareTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ private static void UseNotImplemented(IApplicationBuilder app)
[Fact]
public void NullArguments_ArgumentNullException()
{
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();
Copy link
Contributor

@dougbu dougbu Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

@pranavkm pranavkm Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Tratcher, what do you think? https://github.com/dotnet/aspnetcore/blob/master/src/Http/Http/src/Builder/ApplicationBuilder.cs#L22

Copy link
Contributor Author

@pranavkm pranavkm Jun 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I can do that in a follow up. Let's get this PR in

Copy link
Member

@Tratcher Tratcher Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API is very inconsistent.

  1. Adding a parameterless constructor would simplify usage in tests.
  2. ApplicationServices_Get should return a static no-op instance rather than null.
  3. ServerFeatures should also return a read-only no-op instance.

Copy link
Contributor Author

@pranavkm pranavkm Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Captured this in a separate feedback. #23232.

var noOptions = new MapOptions();
Assert.Throws<ArgumentNullException>(() => builder.Map("/foo", configuration: null!));
Assert.Throws<ArgumentNullException>(() => new MapMiddleware(noMiddleware, null!));
Expand All @@ -57,7 +57,7 @@ public void NullArguments_ArgumentNullException()
public async Task PathMatchFunc_BranchTaken(string matchPath, string basePath, string requestPath)
{
HttpContext context = CreateRequest(basePath, requestPath);
var builder = new ApplicationBuilder(serviceProvider: null);
var builder = new ApplicationBuilder(serviceProvider: null!);
builder.Map(matchPath, UseSuccess);
var app = builder.Build();
await app.Invoke(context);
Expand Down Expand Up @@ -85,7 +85,7 @@ public async Task PathMatchFunc_BranchTaken(string matchPath, string basePath, s
public async Task PathMatchAction_BranchTaken(string matchPath, string basePath, string requestPath)
{
HttpContext context = CreateRequest(basePath, requestPath);
var builder = new ApplicationBuilder(serviceProvider: null);
var builder = new ApplicationBuilder(serviceProvider: null!);
builder.Map(matchPath, subBuilder => subBuilder.Run(Success));
var app = builder.Build();
await app.Invoke(context);
Expand Down Expand Up @@ -113,7 +113,7 @@ public async Task PathMatchAction_BranchTaken(string matchPath, string basePath,
public async Task PathMatchAction_BranchTaken_WithPreserveMatchedPathSegment(string matchPath, string basePath, string requestPath)
{
HttpContext context = CreateRequest(basePath, requestPath);
var builder = new ApplicationBuilder(serviceProvider: null);
var builder = new ApplicationBuilder(serviceProvider: null!);
builder.Map(matchPath, true, subBuilder => subBuilder.Run(Success));
var app = builder.Build();
await app.Invoke(context);
Expand All @@ -129,7 +129,7 @@ public async Task PathMatchAction_BranchTaken_WithPreserveMatchedPathSegment(str
[InlineData("/foo/cho/")]
public void MatchPathWithTrailingSlashThrowsException(string matchPath)
{
Assert.Throws<ArgumentException>(() => new ApplicationBuilder(serviceProvider: null).Map(matchPath, map => { }).Build());
Assert.Throws<ArgumentException>(() => new ApplicationBuilder(serviceProvider: null!).Map(matchPath, map => { }).Build());
}

[Theory]
Expand All @@ -143,7 +143,7 @@ public void MatchPathWithTrailingSlashThrowsException(string matchPath)
public async Task PathMismatchFunc_PassedThrough(string matchPath, string basePath, string requestPath)
{
HttpContext context = CreateRequest(basePath, requestPath);
var builder = new ApplicationBuilder(serviceProvider: null);
var builder = new ApplicationBuilder(serviceProvider: null!);
builder.Map(matchPath, UseNotImplemented);
builder.Run(Success);
var app = builder.Build();
Expand All @@ -165,7 +165,7 @@ public async Task PathMismatchFunc_PassedThrough(string matchPath, string basePa
public async Task PathMismatchAction_PassedThrough(string matchPath, string basePath, string requestPath)
{
HttpContext context = CreateRequest(basePath, requestPath);
var builder = new ApplicationBuilder(serviceProvider: null);
var builder = new ApplicationBuilder(serviceProvider: null!);
builder.Map(matchPath, UseNotImplemented);
builder.Run(Success);
var app = builder.Build();
Expand All @@ -179,7 +179,7 @@ public async Task PathMismatchAction_PassedThrough(string matchPath, string base
[Fact]
public async Task ChainedRoutes_Success()
{
var builder = new ApplicationBuilder(serviceProvider: null);
var builder = new ApplicationBuilder(serviceProvider: null!);
builder.Map("/route1", map =>
{
map.Map("/subroute1", UseSuccess);
Expand Down
12 changes: 6 additions & 6 deletions src/Http/Http.Abstractions/test/MapPredicateMiddlewareTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ private bool FalsePredicate(HttpContext context)
[Fact]
public void NullArguments_ArgumentNullException()
{
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();
var noOptions = new MapWhenOptions();
Assert.Throws<ArgumentNullException>(() => builder.MapWhen(null!, UseNotImplemented));
Assert.Throws<ArgumentNullException>(() => builder.MapWhen(NotImplementedPredicate, configuration: null!));
Expand All @@ -63,7 +63,7 @@ public void NullArguments_ArgumentNullException()
public async Task PredicateTrue_BranchTaken()
{
HttpContext context = CreateRequest();
var builder = new ApplicationBuilder(serviceProvider: null);
var builder = new ApplicationBuilder(serviceProvider: null!);
builder.MapWhen(TruePredicate, UseSuccess);
var app = builder.Build();
await app.Invoke(context);
Expand All @@ -75,7 +75,7 @@ public async Task PredicateTrue_BranchTaken()
public async Task PredicateTrueAction_BranchTaken()
{
HttpContext context = CreateRequest();
var builder = new ApplicationBuilder(serviceProvider: null);
var builder = new ApplicationBuilder(serviceProvider: null!);
builder.MapWhen(TruePredicate, UseSuccess);
var app = builder.Build();
await app.Invoke(context);
Expand All @@ -87,7 +87,7 @@ public async Task PredicateTrueAction_BranchTaken()
public async Task PredicateFalseAction_PassThrough()
{
HttpContext context = CreateRequest();
var builder = new ApplicationBuilder(serviceProvider: null);
var builder = new ApplicationBuilder(serviceProvider: null!);
builder.MapWhen(FalsePredicate, UseNotImplemented);
builder.Run(Success);
var app = builder.Build();
Expand All @@ -99,7 +99,7 @@ public async Task PredicateFalseAction_PassThrough()
[Fact]
public async Task ChainedPredicates_Success()
{
var builder = new ApplicationBuilder(serviceProvider: null);
var builder = new ApplicationBuilder(serviceProvider: null!);
builder.MapWhen(TruePredicate, map1 =>
{
map1.MapWhen((Predicate)FalsePredicate, UseNotImplemented);
Expand Down
4 changes: 2 additions & 2 deletions src/Http/Http.Abstractions/test/UsePathBaseExtensionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public IServiceProvider ApplicationServices
set { _wrappedBuilder.ApplicationServices = value; }
}

public IDictionary<string, object> Properties => _wrappedBuilder.Properties;
public IDictionary<string, object?> Properties => _wrappedBuilder.Properties;
public IFeatureCollection ServerFeatures => _wrappedBuilder.ServerFeatures;
public RequestDelegate Build() => _wrappedBuilder.Build();
public IApplicationBuilder New() => _wrappedBuilder.New();
Expand Down Expand Up @@ -163,7 +163,7 @@ private static HttpContext CreateRequest(string pathBase, string requestPath)

private static ApplicationBuilder CreateBuilder()
{
return new ApplicationBuilder(serviceProvider: null);
return new ApplicationBuilder(serviceProvider: null!);
}
}
}
2 changes: 1 addition & 1 deletion src/Http/Http.Abstractions/test/UseWhenExtensionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ private static HttpContext CreateContext()

private static ApplicationBuilder CreateBuilder()
{
return new ApplicationBuilder(serviceProvider: null);
return new ApplicationBuilder(serviceProvider: null!);
}

private static bool TruePredicate(HttpContext context)
Expand Down