Skip to content
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.

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

Microsoft.AspNetCore.Identity.Specification.Tests.IdentitySpecificationTestBase tests failed since .Net 5.0 #27873

Closed
aguacongas opened this issue Nov 16, 2020 · 16 comments
Assignees
Labels
area-identity Includes: Identity and providers bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed Servicing-consider Shiproom approval is required for the issue
Milestone

Comments

@aguacongas
Copy link
Contributor

Describe the bug

I use Microsoft.AspNetCore.Identity.Specification.Tests in my CI for Identity.Redis and Identity.Firebase but since I upgraded to .Net 5.0 some tests failed because the new UserManager return the user id in errors messages and it wasn't the case before.

To Reproduce

  • Implement a UserStore
  • Test it with Microsoft.AspNetCore.Identity.Specification.Tests.IdentitySpecificationTestBase

Exceptions (if any)

For exemple:

Assert.Contains() Failure
Not found: User f137e1ad-52cd-4f8b-9b19-9a1e1f89f721 is locked out.
In value:  List<String> ["User is locked out."]

The list of failed tests can be found here : https://ci.appveyor.com/project/aguacongas/identity-redis/build/job/xif5iryfcmy965v5/tests or https://ci.appveyor.com/project/aguacongas/identity-firebase/builds/36329110/job/51f21lutxhdotu28/tests

Further technical details

  • ASP.NET Core version 5.0
  • Include the output of dotnet --info
    .NET SDK (reflecting any global.json):
    Version: 5.0.100
    Commit: 5044b93829

Runtime Environment:
OS Name: Windows
OS Version: 10.0.19041
OS Platform: Windows
RID: win10-x64
Base Path: C:\Program Files\dotnet\sdk\5.0.100\

Host (useful for support):
Version: 5.0.0
Commit: cf258a14b7

.NET SDKs installed:
2.2.207 [C:\Program Files\dotnet\sdk]
3.1.300-preview-015095 [C:\Program Files\dotnet\sdk]
3.1.300 [C:\Program Files\dotnet\sdk]
5.0.100 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
Microsoft.AspNetCore.All 2.1.23 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.All 2.2.8 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
Microsoft.AspNetCore.App 2.1.23 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 2.2.8 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 3.1.4 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 3.1.9 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 5.0.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.NETCore.App 2.1.23 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 2.2.8 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 3.1.4 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 3.1.9 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 5.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.WindowsDesktop.App 3.1.4 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 3.1.9 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 5.0.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

To install additional .NET runtimes or SDKs:
https://aka.ms/dotnet-download

@blowdart
Copy link
Contributor

@HaoK when we stripped the ID, did tests not get updated? Seems odd it didn't break for us.

@blowdart blowdart added area-identity Includes: Identity and providers test-failure labels Nov 16, 2020
@HaoK
Copy link
Member

HaoK commented Nov 16, 2020

Looks like we stopped shipping this package in 5.0, because it depended on an internal MVC testing package (for flaky tests to work). This PR removed the isshipping 2d066dc#diff-f9587d7bcf8224c6b23f8d096a59c226d3175f6b126e9953f742da4d5eb96a0d

So yeah, using 5.0 identity bits against the 3.0 specification tests would result in these kinds of errors. @aguacongas can you confirm this is the issue you are running into?

@aguacongas
Copy link
Contributor Author

@HaoK actually I don't really know if its because you stopped shipping this package or not. Packages versions used are :

<PackageReference Include="Microsoft.AspNetCore.Identity.Specification.Tests" Version="3.1.10" />
<PackageReference Include="Microsoft.AspNetCore.Identity" Version="2.2.0" />
<PackageReference Include="Microsoft.Extensions.Identity.Stores" Version="5.0.0" />

@HaoK
Copy link
Member

HaoK commented Nov 17, 2020

That's a weird mix of versions, can you try using 3.1.10 for all 3 to verify that things work with consistent versions?

@aguacongas
Copy link
Contributor Author

2.2.0 is latest version of Microsoft.AspNetCore.Identity. It was working with Microsoft.Extensions.Identity.Stores and Microsoft.AspNetCore.Identity.Specification.Tests 3.1.10

@HaoK
Copy link
Member

HaoK commented Nov 17, 2020

Identity is part of the shared framework now, so you won't see the packages the same way you should be using them that way

@aguacongas
Copy link
Contributor Author

ok, I can remove Microsoft.AspNetCore.Identity dependency, it still compile.
If I use Microsoft.Extensions.Identity.Stores 3.1.10 and Microsoft.AspNetCore.Identity.Specification.Tests 3.1.10 and the test project target framework is netcoreapp3.1, it works, tests don't failed.
If I use Microsoft.Extensions.Identity.Stores 5.0.0 or if the test project target framework is net5.0, tests fail.

@HaoK HaoK removed the test-failure label Nov 17, 2020
@HaoK
Copy link
Member

HaoK commented Nov 17, 2020

Removing test failure label @blowdart as this isn't actually a issue with the tests, its due to versioning / no longer shipping a 5.0 version of these shared tests so there's no way to continue testing in 5.0 using the old packages due to behavior differences. We stopped shipping these due to the quarantine attribute, we might need to revisit this and see if we can come up with something different to continue shipping the specification tests and quarantining the tests a different way

@blowdart
Copy link
Contributor

Yea, if folks are using these as a way to ensure that their own store implementations work, we make the tests available and keep them up to date.

@HaoK
Copy link
Member

HaoK commented Nov 17, 2020

I think the fix will just be to stop using the attribute in the specification assembly, and explicitly override add the quarantine attribute via overriding any flaky tests in every test assembly since those are not shipped

@blowdart
Copy link
Contributor

Plus update the tests to reflect the removal of PII from logs

@HaoK
Copy link
Member

HaoK commented Nov 17, 2020

The tests are fine, otherwise our CI are broken. The issue is running 5.0 product code against 3.x tests since the 5.0 tests aren't public anymore

@blowdart
Copy link
Contributor

Ah. OK, well, as long as folks can test and be reassured we didn't break them :)

@HaoK
Copy link
Member

HaoK commented Nov 17, 2020

Yeah so that's no longer the case in 5.0 right now, other than copying the source from our 5.0 branch into their repo

@blowdart
Copy link
Contributor

Yes ... so can we make the tests public again?

@mkArtakMSFT mkArtakMSFT added test-failure bug This issue describes a behavior which is not expected - a bug. and removed test-failure labels Nov 20, 2020
@mkArtakMSFT mkArtakMSFT added this to the 5.0.1 milestone Nov 20, 2020
@mkArtakMSFT mkArtakMSFT added the Servicing-consider Shiproom approval is required for the issue label Nov 20, 2020
@ghost ghost added Done This issue has been fixed and removed Working labels Nov 20, 2020
@JohnCampionJr
Copy link

Will this new 5.0.1 version get published to NuGet soon?

@ghost ghost locked as resolved and limited conversation to collaborators Jan 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-identity Includes: Identity and providers bug This issue describes a behavior which is not expected - a bug. Done This issue has been fixed Servicing-consider Shiproom approval is required for the issue
Projects
None yet
Development

No branches or pull requests

5 participants