Allow CollectionAttribute to reference a definition class directly

[JamesTerwilliger]

Attempted fix for #2629

This PR allows CollectionAttribute some flexibility in referencing a collection definition/fixture. It gives two new options:

• Take a class type reference rather than a string as a collection identifier, and
• Even "better"/"newer", allow CollectionAttribute<TCollectionDefinition> as a generic way to also reference the fixture class

More tests to come

[bradwilson]

I haven't really looked at the tests yet, as I wanted to see the impact on the production code first, and a few more things popped out to me as I did that. Thoughts?

[bradwilson]

I'm going to add tests for the ReflectionAttributeInfo changes, as well as porting the changes into ReflectionAssemblyInfo, ReflectionMethodInfo, ReflectionParameterInfo, and ReflectionTypeInfo.

[JamesTerwilliger]

Awesome! I'll look into making changes to the analyzer to compensate once this goes in.

[bradwilson]

This is becoming...a large change to get this all working the way I want. I think I might end up doing it in the context of the main branch (since it's a substantial change by itself) and then I'll merge it in once I'm done.

[JamesTerwilliger]

Understood.

[bradwilson]

This is the commit: 02175c7

Showing 38 changed files with 935 additions and 305 deletions.

Yeah. 😂

I'll get it merged and resolved, since I stepped on you a little bit, and see how everything is playing out after that.

[JamesTerwilliger]

Showing 38 changed files with 935 additions and 305 deletions.

MotherOfGod.jpg

[bradwilson]

I've got a flaky test situation going on. I'm going to push the merge but I'm still looking through things to see if I can figure out where the problem is.

[JamesTerwilliger]

By any chance is the flaky test MessageBusTests.QueueReturnsFalseForFailIfStopOnFailTrue?

[bradwilson]

It's definitely somehow related to caching, and I didn't just introduce it now, because running back a couple steps into your history was broken.

I had to make some surgical changes because it wouldn't fail just run by itself (being cache related) so I updated _TestFailed to print its messages and tweaked the failing tests a little so I could see them, and this is what I'm seeing:

There are three tests that I see that fail, and which one fails is a bit random because of the randomization of test runs.

The classes under test look like this:

[Collection(typeof(CollectionWithClassFixtureCounter))]
class ClassWithCountedFixture : IClassFixture<CountedFixture>
{
	public ClassWithCountedFixture(CountedFixture fixture)
	{
		// 1 == test collection, 2 == test class
		Assert.Equal(2, fixture.Identity);
	}

	[Fact]
	public void TheTest() { }
}

class CountedFixture
{
	static int counter = 0;

	public CountedFixture() => Identity = ++counter;

	public readonly int Identity;
}

I suspect it's because you copied the original test two times, but they're all sharing the same static state. Testing that theory now...

[bradwilson]

Yep, that's it. I just need to move a copy of the CountedFixture up into each place where it's used so they each have their own static state and not one shared static state for all 3. :)

[JamesTerwilliger]

Oof. Sorry. Thanks for catching that. That would explain why the tests were passing in isolation when I ran them.

It's odd, I had no trouble getting this test to get picked up by the test runner, but I'm not able to get one to show up on a different change I'm making that's also blocked on .Net 7 or higher.

I'll take a crack at an analyzer for this issue next once this gets merged.

[bradwilson]

As an aside, I was able to use generic attributes in my tests for all of .NET Framework 4.7.2. and .NET 6. I don't think .NET 8 is necessary for those. All I needed to make sure was that I had access to C# 11. Example:

https://github.com/xunit/xunit/blob/main/src/xunit.v3.core.tests/Sdk/Reflection/ReflectionAssemblyInfoTests.cs

[JamesTerwilliger]

Ooh, nice! The other item I was looking at is static interface virtual methods, which I think does require .Net 7 or higher sadly.

[bradwilson]

Yeah, there's no need for me to roll back the .NET 8 test support, it'll eventually be needed. I just don't think we need the guards on these tests.

[bradwilson]

Well, I ran into a problem.

The tests for Collection<T> work fine on .NET 6 as expected. However, they fail on .NET Framework, presumably because it cannot find generic attributes at all, even when using C# 11 to define them. That means ideally we should not offer Collection<T> to .NET Framework users; however, it's defined in xunit.v3.core which targets netstandard2.0, not net472 and/or net6.0.

I'm super reluctant to diverge the code base of xunit.v3.core, so I'm going to sit on this for a bit and ponder (and check in the few changes I've already made).

[JamesTerwilliger]

Oof. Yeah, that's a problem, and I don't have any immediate ideas either.

[bradwilson]

One option is to keep [Collection(typeof(T))] and remove [Collection<T>]. That doesn't fit the exact request, but it does provide some benefit over just using strings (including the ability to use a collection definition that exists in a different assembly, something which we don't support today because of the cost of scanning the assembly for collection definition classes).

[JamesTerwilliger]

One option is to keep [Collection(typeof(T))] and remove [Collection<T>]. That doesn't fit the exact request, but it does provide some benefit over just using strings (including the ability to use a collection definition that exists in a different assembly, something which we don't support today because of the cost of scanning the assembly for collection definition classes).

At the very least, we could split the PR in two and check in the typeof(T) now while figuring out the other half.

[JamesTerwilliger]

Does making Collection<T> derive from Collection(typeof(T)) simplify anything? At the moment, CollectionAttribute is sealed, which is why I didn't do that to begin with, but it may be a way out.

[bradwilson]

Unfortunately that means unsealing CollectionAttribute, which isn't an option. We seal attributes that we find via reflection because our reflection abstractions may be running where there are no binaries available (for example, to do test discovery via source code with something like Roslyn or Resharper). If the attribute is sealed, then we know for certain how it's constructed and what's available; if it's not sealed, then we don't know what other people may have done in their derivations.

I'll poke around with reflection in .NET Framework and see if I can understand what's going on, but I'm not super hopeful that I'm going to find a magic way out.

[bradwilson]

Hmm, interesting.

using System;
using Xunit;

namespace Empty;

class Attribute1<T> : Attribute { }

[Attribute1<TestClass>]
public class TestClass
{
	[Fact]
	public void TestMethod()
	{
		var attributes = typeof(TestClass).CustomAttributes;

		Assert.Empty(attributes);
	}
}

Results in:

.NET Framework:

Assert.Empty() Failure: Collection was not empty
Collection: [[Empty.Attribute1`1[[System.__Canon, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089]]()]]

.NET 6:

Assert.Empty() Failure: Collection was not empty
Collection: [[Empty.Attribute1`1[[Empty.TestClass, empty, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null]]()]]

So it's there, but the type is mangled somehow.

[JamesTerwilliger]

Reason number 420 why I'll be glad when Framework is finally put out to pasture, that's absolutely maddening. :-(

[bradwilson]

This tends to imply that (a) runtime support was required to make generic attributes work, and (b) such runtime support was only added to .NET Core, and not .NET Framework.

Not a solid confirmation, but pretty strongly suggestive.

[bradwilson]

Oddly enough it works correctly if you use a builtin type (though that of course isn't helpful to us).

.NET Framework:
Collection: [[Empty.Attribute1`1[[System.Int32, mscorlib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089]]()]]

.NET 6:
Collection: [[Empty.Attribute1`1[[System.Int32, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]]()]]

[bradwilson]

I'm going to see if I can make this work with reference assemblies. I might be able to multi-target the build of xunit.v3.core but only ship the netstandard2.0 binary in the NuGet package (but include both reference libraries, where the .NET Framework version does not include the generic attributes).

[bradwilson]

Local package creation and testing looked good. Intellisense and references looked correct; CollectionAttribute<T> was not visible in .NET Framework, and none of the XML docs reference it either. The built binary includes the sole lib/netstandard2.0 binary when compiling for either .NET Framework 4.7.2 or .NET 6. So... I think maybe this is gonna be good? 🤞🏼

[bradwilson]

I think this is ready for your review if you'd like, @JamesTerwilliger. I'll merge if you're satisfied.

[JamesTerwilliger]

@bradwilson I'm looking now, but it looks as if the PR builds are failing still (non-windows only?)

[bradwilson]

Fixing the non-Windows issue right now.

[JamesTerwilliger]

Found and fixed one spelling error, code itself looks good (well, good is relative given all of the ID mangling magic that needed to happen in a couple of places, but the logic looks sound) @bradwilson #ShipIt

[bradwilson]

Once the packages are pushed to feedz.io I'll do one last test to ensure things are working as expected. Don't think I should see any surprises here but you never know. 😂

[bradwilson]

Woot! Doing its thing!

[JamesTerwilliger]

@bradwilson Nicely done, sir!