Skip to content

Adding logging to identify the issue in https://github.com/aspnet/AspNetCore-Internal/issues/1803 #8102

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 2 commits into from
Mar 5, 2019
Merged

Adding logging to identify the issue in https://github.com/aspnet/AspNetCore-Internal/issues/1803 #8102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3576418
Revert "Skipping flaky test"
pranavkm Mar 1, 2019
e340105
Add additional logging to TempDataInCookiesTest to diagnose test failure
pranavkm Mar 1, 2019
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
38 changes: 37 additions & 1 deletion src/Mvc/test/Mvc.FunctionalTests/TempDataInCookiesTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,23 +6,59 @@
using System.Linq;
using System.Net;
using System.Net.Http;
using System.Text;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Authentication.Cookies;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Mvc.ViewFeatures;
using Microsoft.AspNetCore.Mvc.ViewFeatures.Infrastructure;
using Microsoft.AspNetCore.TestHost;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Net.Http.Headers;
using Xunit;

namespace Microsoft.AspNetCore.Mvc.FunctionalTests
{
public class TempDataInCookiesTest : TempDataTestBase, IClassFixture<MvcTestFixture<BasicWebSite.StartupWithoutEndpointRouting>>
{
private IServiceCollection _serviceCollection;

public TempDataInCookiesTest(MvcTestFixture<BasicWebSite.StartupWithoutEndpointRouting> fixture)
{
Client = fixture.CreateDefaultClient();
var factory = fixture.Factories.FirstOrDefault() ?? fixture.WithWebHostBuilder(b => b.UseStartup<BasicWebSite.StartupWithoutEndpointRouting>());
factory = factory.WithWebHostBuilder(b => b.ConfigureTestServices(serviceCollection => _serviceCollection = serviceCollection));

Client = factory.CreateDefaultClient();
}

protected override HttpClient Client { get; }

[Fact]
public void VerifyNewtonsoftJsonTempDataSerializer()
{
// Arrange
// This test could provide some diagnostics for the test failure reported in https://github.com/aspnet/AspNetCore-Internal/issues/1803.
// AddNewtonsoftJson attempts to replace the DefaultTempDataSerializer. The test failure indicates this failed but it's not clear why.
// We'll capture the application's ServiceCollection and inspect the instance of ITempDataSerializer instances here. It might give us some
// clues if the test fails again in the future.

// Intentionally avoiding using Xunit.Assert to get more diagnostics.
var tempDataSerializers = _serviceCollection.Where(f => f.ServiceType == typeof(TempDataSerializer)).ToList();
if (tempDataSerializers.Count == 1 && tempDataSerializers[0].ImplementationType.FullName == "Microsoft.AspNetCore.Mvc.NewtonsoftJson.BsonTempDataSerializer")
{
return;
}

var builder = new StringBuilder();
foreach (var serializer in tempDataSerializers)
{
var type = serializer.ImplementationType;
builder.Append(serializer.ImplementationType.AssemblyQualifiedName);
}

throw new Exception($"Expected exactly one instance of TempDataSerializer based on NewtonsoftJson, but found {tempDataSerializers.Count} instance(s):" + Environment.NewLine + builder);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

Copy link
Contributor

Choose a reason for hiding this comment

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

If this fails, it might get reported as a new issue since the callstack will be different. You might need to watch for new incoming issues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The plan is to not merge this in to master. Just re-running it here to help diagnose the issue

Copy link
Contributor

Choose a reason for hiding this comment

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

Using a Draft PR might make that clearer (you can't go back to Draft now though so 🤷‍♂️)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😿 I was actually planning on checking in before I checked in with you (🥁).

Copy link
Contributor

Choose a reason for hiding this comment

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

Bill Nye gasping


[Theory]
[InlineData(ChunkingCookieManager.DefaultChunkSize)]
[InlineData(ChunkingCookieManager.DefaultChunkSize * 1.5)]
Expand Down
18 changes: 9 additions & 9 deletions src/Mvc/test/Mvc.FunctionalTests/TempDataTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ public abstract class TempDataTestBase
{
protected abstract HttpClient Client { get; }

[Fact(Skip = "https://github.com/aspnet/AspNetCore-Internal/issues/1803")]
[Fact]
public async Task PersistsJustForNextRequest()
{
// Arrange
Expand Down Expand Up @@ -47,7 +47,7 @@ public async Task PersistsJustForNextRequest()
Assert.Equal(HttpStatusCode.NoContent, response.StatusCode);
}

[Fact(Skip = "https://github.com/aspnet/AspNetCore-Internal/issues/1803")]
[Fact]
public async Task ViewRendersTempData()
{
// Arrange
Expand All @@ -66,7 +66,7 @@ public async Task ViewRendersTempData()
Assert.Equal("Foo", body);
}

[Fact(Skip = "https://github.com/aspnet/AspNetCore-Internal/issues/1803")]
[Fact]
public async Task Redirect_RetainsTempData_EvenIfAccessed()
{
// Arrange
Expand Down Expand Up @@ -97,7 +97,7 @@ public async Task Redirect_RetainsTempData_EvenIfAccessed()
Assert.Equal("Foo", body);
}

[Fact(Skip = "https://github.com/aspnet/AspNetCore-Internal/issues/1803")]
[Fact]
public async Task Peek_RetainsTempData()
{
// Arrange
Expand Down Expand Up @@ -130,7 +130,7 @@ public async Task Peek_RetainsTempData()
Assert.Equal("Foo", body);
}

[Fact(Skip = "https://github.com/aspnet/AspNetCore-Internal/issues/1803")]
[Fact]
public async Task ValidTypes_RoundTripProperly()
{
// Arrange
Expand Down Expand Up @@ -162,7 +162,7 @@ public async Task ValidTypes_RoundTripProperly()
Assert.Equal($"Foo 10 3 10/10/2010 00:00:00 {testGuid.ToString()}", body);
}

[Fact(Skip = "https://github.com/aspnet/AspNetCore-Internal/issues/1803")]
[Fact]
public async Task ResponseWrite_DoesNotCrashSaveTempDataFilter()
{
// Arrange
Expand All @@ -176,7 +176,7 @@ public async Task ResponseWrite_DoesNotCrashSaveTempDataFilter()
var response = await Client.GetAsync("/TempData/SetTempDataResponseWrite");
}

[Fact(Skip = "https://github.com/aspnet/AspNetCore-Internal/issues/1803")]
[Fact]
public async Task SetInActionResultExecution_AvailableForNextRequest()
{
// Arrange
Expand Down Expand Up @@ -207,7 +207,7 @@ public async Task SetInActionResultExecution_AvailableForNextRequest()
Assert.Equal(HttpStatusCode.NoContent, response.StatusCode);
}

[Fact(Skip = "https://github.com/aspnet/AspNetCore-Internal/issues/1803")]
[Fact]
public async Task SaveTempDataFilter_DoesNotSaveTempData_OnUnhandledException()
{
// Arrange & Act
Expand All @@ -225,7 +225,7 @@ public async Task SaveTempDataFilter_DoesNotSaveTempData_OnUnhandledException()
Assert.Equal(HttpStatusCode.NoContent, response.StatusCode);
}

[Fact(Skip = "https://github.com/aspnet/AspNetCore-Internal/issues/1803")]
[Fact]
public async Task SaveTempDataFilter_DoesNotSaveTempData_OnHandledExceptions()
{
// Arrange & Act
Expand Down