aspnet / MusicStore

[Archived] MusicStore test application that uses ASP.NET/EF Core. Project moved to https://github.com/aspnet/AspNetCore
1.3k stars 878 forks source link

Clean up E2E tests #601

Closed natemcmaster closed 8 years ago

natemcmaster commented 8 years ago

Should fix failing tests. Currently on CI the agents are all using the same database, across concurrent multiple tests. In order to figure out what was wrong, I had to fix these other things to make it easier to diagnose what was going wrong.

cc @kichalla @pranavkm

natemcmaster commented 8 years ago

@kichalla There is one test still failing in this PR. I can't quite figure out what is wrong. The test fails on RegisterUserWithNonMatchingPasswords. Instead of getting the page with an error, it receives a redirect to /home/statuscodepage. Any ideas?

See https://ci.appveyor.com/project/aspnetci/musicstore/build/1.0.597#L6691

kichalla commented 8 years ago

@natemcmaster investigatng it now

kichalla commented 8 years ago

There is something going on. I tried to repro this method standalone (a dotnet new app with the above method and its helpers hitting a deployed musicstore app), but it works fine. Investigating...

kichalla commented 8 years ago

The issue that I am seeing is that the antiforgery cookie is not being sent back to the server which causes the antiforgery validation to fail causing a 400 Bad Request response, which in turn is causing the StatusCodePage middleware to get executed.

I'm seeing that the client is indeed receiving the antiforgery token cookie, but doesn't send this again in the next request (but it sends the session cookie though).

Response from server:

Response headers:StatusCode: 200, ReasonPhrase: 'OK', Version: 1.1, Content: System.Net.Http.StreamContent, Headers:
      {
        Date: Sun, 06 Mar 2016 18:21:26 GMT
        Transfer-Encoding: chunked
        Server: Kestrel
        Set-Cookie: .AspNetCore.Antiforgery.DLAROKrWNuQ=CfDJ8NGNxAt7CbdClq3UJ8_6w_7yQ6tKntRlX7M6POCPx9mETGzkNfF2b0KhP6YX5xoptXKOMn37uXe1tF6i9Q7vvcaYiIFyTZCMQHeY01ob6x_SX1tVfj8Eo17mTnO3dm5nvGehjNYa1ldcLmAmbr4iAC4; path=/; httponly
        X-Frame-Options: SAMEORIGIN
        Content-Type: text/html; charset=utf-8
      }

Now when I query the HttpClientHandler for the cookie list, I only get the following:

ClientHandler cookies---------------->.AspNetCore.Session : 4aeb3f5f-5cea-dcf6-7c93-c78e43fe465d

Now a request made to the server and intercepted by a middleware(registered first in the pipeline), as expected, sees only the session cookie:

     *********************************Request Cookies*********************************
info: Smoke:Kestrel:CoreClr:x64[0]
      .AspNetCore.Session: 4aeb3f5f-5cea-dcf6-7c93-c78e43fe465d

I tried to repro this issue outside MusicStore by creating a simple web api app and using CoreClr httpclient(of the exact version as musicstore), but i cannot repro it. I am kind of stumped now.

@Tratcher Any pointers?

natemcmaster commented 8 years ago

What's weirder is this error does not reproduce on Linux or OSX.

Could this be a bug in HttpClientHandler and httponly cookies?

natemcmaster commented 8 years ago

Aha: https://github.com/dotnet/corefx/blob/61ff821f213cf323bd52aad3623f9204901311e7/src/System.Net.Http/src/netcore50/System/Net/HttpClientHandler.cs#L462

kichalla commented 8 years ago

Thanks @natemcmaster . I was also suspecting if this is a problem with httpclienthandler, but following does not explain though:

natemcmaster commented 8 years ago

@kichalla it does seem weird, but I'm partially done with a fix that saves httponly cookies in our smoke test and it appears to fix the failure. (Btw, there are more bugs that have been hiding behind this one...more to come)

natemcmaster commented 8 years ago

@kichalla Updated this PR. I believe I've fixed the flaky smoke tests. It also appears to have uncovered other bugs. #602 and potentially a bug in CoreCLR (I'm investigating futher: for now the workaround is to explicitly save httponly cookies.)

kichalla commented 8 years ago

if the test passes, :shipit: and Thanks!

Regarding identity cookie name, I believe @JunTaoLuo was doing some cookie renames across bunch of repos. Probably this needs to be updated.

natemcmaster commented 8 years ago

Cref: https://github.com/dotnet/corefx/issues/6728

kichalla commented 8 years ago

@natemcmaster Was looking at the repro code. The two have the same cookie name here. Typo? https://gist.github.com/natemcmaster/e51aa33cf77416772afc#file-program-cs-L26

natemcmaster commented 8 years ago

Aha, yes typo. :facepalm:

kichalla commented 8 years ago

@natemcmaster Curious, so does it still repro with the fix? (I am kind of surprised how I see it work in some scenarios :confused: ). Also I would suggest to provide the lock.json in the repro so that they can repro it with the exact version you are seeing.

natemcmaster commented 8 years ago

No, the test app doesn't repro with the cookie name fixed. Which leads me to believe the HttpOnly problem is not the real issue. I may close that corefx bug.

natemcmaster commented 8 years ago

:watch: https://github.com/aspnet/Identity/pull/764

kichalla commented 8 years ago

I was finally able to repro the cookie issue standalone: https://github.com/dotnet/corefx/issues/6737