Skip to content

Cannot run tests in parallel because TypeLocator is using a static cache shared between tests #485

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

Closed
idoban opened this issue Mar 13, 2019 · 3 comments · Fixed by #700
Closed

Comments

@idoban
Copy link

idoban commented Mar 13, 2019

Description

Cannot run concurrent tests using TestServer when using Auto Discovery to register types.

Staetup.cs
services.AddJsonApi(facade => facade.AddCurrentAssembly());

System.InvalidOperationException : Collection was modified; enumeration operation may not execute.
Stack Trace:
at System.Collections.Generic.List`1.Enumerator.MoveNextRare()
at JsonApiDotNetCore.Graph.ServiceDiscoveryFacade.AddAssembly(Assembly assembly) in jsonapidotnetcore\src\JsonApiDotNetCore\Graph\ServiceDiscoveryFacade.cs:line 76

The root cause is due to
private static Dictionary<Assembly, List> _identifiableTypeCache = new Dictionary<Assembly, List>();

which is being concurrently updated by different tests (each starting up its own TestServer for tests isolation).

see:

private static Dictionary<Assembly, List<ResourceDescriptor>> _identifiableTypeCache = new Dictionary<Assembly, List<ResourceDescriptor>>();

Environment

  • JsonApiDotNetCore Version: 3.1.0
  • Other Relevant Package Versions:
@maurei
Copy link
Member

maurei commented Apr 29, 2019

I agree it's not optimal to not be able to run the test concurrently, but they still run pretty quick so it's not really bothering me, therefore I would say this is a pretty low priority issue. Thoughts?

@jaredcnance
Copy link
Contributor

jaredcnance commented Apr 29, 2019

@idoban can you patch it to use ConcurrentDictionary and see if it solves your problem? If it does, then you can open a PR.

@bart-degreed
Copy link
Contributor

@idoban Can you verify if #700 works for you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

4 participants