Fix for https://github.com/xunit/xunit/issues/2557

[JamesTerwilliger]

This PR is a candidate fix for #2557, allowing CollectionFixture objects to be generic at the point of CollectionAttribute specification.

The main reason why the PR is as large as it is, is because the "instantiated" version of the generic collection fixture isn't known as early as the actual generic version that is marked with the actual attribute. As such, it has to lazily create and squirrel away the instance. If I am missing something that would allow earlier instantiation, that would make this PR simpler.

[JamesTerwilliger]

@bradwilson would very much appreciate a small bit of guidance as to whether this approach works or if something else might be better.

[bradwilson]

Thanks! It'll be a couple days before I can get a look at this.

[bradwilson]

This seems like a good start. I'm going to make some changes to get things to build & test properly as well to update for our coding style.

[bradwilson]

I noticed the async initialization code isn't properly awaiting for the result of InitializeAsync so I'm going to need to restructure things a little bit.

[JamesTerwilliger]

I noticed the async initialization code isn't properly awaiting for the result of InitializeAsync so I'm going to need to restructure things a little bit.

Oof. That does kind of defeat part of the purpose of this. I look forward to seeing the edit, apologies.

[bradwilson]

No worries. It's a complex bit of code, especially coordinating to ensure everything gets initialized and cleaned up appropriately. Async is always tricky. 😄

[bradwilson]

This is much trickier than it looks at first, because in theory it needs to track lifetime and initialization status at multiple levels, so it feels like all the instances of Dictionary<Type, object> end up getting replaced by a general purpose fixture mapping manager, but I do think it'll end up cleaning up all the consuming code.

[JamesTerwilliger]

Having recently been shoulder-deep in the fixture code, the thought of a general-purpose fixture mapping manager is indeed very appealing. Please let me know if I can help.

[bradwilson]

I'm poking around with it, starting with your design and then just moving all the lifetime management into the mapper and out of the runners/contexts. It definitely feels like it's going to clean things up, but until it's done I won't know.

Will definitely be interested to having you review it when it's done and see what you think.

[JamesTerwilliger]

Understood, and willing and able to assist however I can including reviewing the awesomeness.

[bradwilson]

So here's what I landed.

FixtureMappingManager is used for all three levels of fixtures: assembly, collection, and class. The context classes hold the appropriate mapping manager, but it's the runner classes which initialize and clean them up. When creating an instance, you give it the parent mapping manager (if there is one) so that it can resolve constructor dependencies up the chain as needed.

The initialization is done by way of calling InitializeAsync with the list of known fixture types. For everything which is concretely creatable, it creates an instance immediately and caches it. For things which aren't concretely creatable (because they're partially closed generics like FixtureClass<T> on a collection fixture definition), we keep track of the fully open type (in this case, FixtureClass<>) and if anybody comes along and asks for a closed version (like FixtureClass<int>), we create it dynamically and cache it.

The runner will call DisposeAsync when it's done with the run, so that all the fixtures can get cleaned up (and any cleanup failures can be tracked and reported as cleanup failures).

All the fixture creation and cleanup code got consolidated into FixtureMappingManager, which is nice because it reduces the duplication of that code across the 3 runners.

Mostly the tests that had to be touched were done so because of the runners context changes.

[bradwilson]

We'll want to make sure xUnit1041 gets updated to understand the new functionality.

[JamesTerwilliger]

I can tackle that next

[bradwilson]

Once you review my changes, I think this PR is ready @JamesTerwilliger

[JamesTerwilliger]

@bradwilson Just finished looking through it, took a while to unwind the nesting in my head but the reduction of redundancy is a massive help. I'd say this looks great, though I had one small nit (commented above) if you want.

[bradwilson]

Thanks! 🎉