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

Support runtimes where ReflectedType exists such as NetStandard2.0 compatible runtimes #2419

Closed
wants to merge 1 commit into from

Conversation

csMACnz
Copy link

@csMACnz csMACnz commented Nov 16, 2021

This relates to issue #1983

The calling code that fails is this particular call in DataDiscoverer:

var reflectionTestMethodType = reflectionTestMethod.Type as IReflectionTypeInfo;

This change should restore the functionality that worked on NetStandard 2.0 compatible runtimes with v2.4.0 that breaks when upgrading to 2.4.1 (where NetStandard2.0 target was removed breaking this particular call.

I think the missing unit tests are possible but still working out where they go into this file. Potentially alongside this test case?

[Fact]
public void CanUseFieldDataFromBaseType()
{
var testMessages = Run<ITestResultMessage>(typeof(ClassWithBaseClassData));
var passed = Assert.Single(testMessages.Cast<ITestPassed>());
Assert.Equal("Xunit2TheoryAcceptanceTests+FieldDataTests+ClassWithBaseClassData.TestViaFieldData(x: 42)", passed.Test.DisplayName);
}
class BaseClass
{
public static IEnumerable<object[]> DataSource = new[] { new object[] { 42 } };
}
class ClassWithBaseClassData : BaseClass
{
[Theory]
[MemberData("DataSource")]
public void TestViaFieldData(int x) { }
}

EDIT: After some playing around I've realised none of these theory tests light up for non .Net Full Framework anyway.

I've tried to do this as low impact as possible.

I investigated just using DeclaringType but there is a subtle difference, which is exactly what the DataDiscoverer is using it for.
(DeclaringType would resolve to the base class while ReflectedType will correctly use the derived test class type, according to Stack Overflow)

Thanks in Advance. All feedback appreciated

@bradwilson
Copy link
Member

Obviously we would want some tests here. I'm fine with the approach of dynamically looking for ReflectedType, though it should be done in a cached lookup since these wrappers get used a lot.

@csMACnz
Copy link
Author

csMACnz commented Jul 26, 2022

Time to actually tackle the tests on this has stalled.

I've found a quick workaround using an alternate DataAttribute custom class though:


    internal class BugFixMemberDataAttribute : MemberDataAttributeBase
    {
        public BugFixMemberDataAttribute(string memberName, params object[] parameters)
            : base(memberName, parameters)
        {
        }

        public override IEnumerable<object[]?>? GetData(MethodInfo testMethod)
        {
            MemberType = testMethod.ReflectedType;
            return base.GetData(testMethod);
        }

        protected override object[]? ConvertDataItem(MethodInfo testMethod, object item)
        {
            if (item == null)
            {
                return null;
            }

            return (item as object[]) ?? throw new ArgumentException($"Property {MemberName} on {MemberType ?? testMethod.DeclaringType} yielded an item that is not an object[]");
        }
    }

YMMV

@bradwilson
Copy link
Member

I wrote the acceptance test for this. I would've pushed the changes into your branch and merged, but you wrote them directly into your v2 branch, which I didn't want to clutter up with my stuff (since it would've required a force push). So your changes are merged here: e813b31

Thanks!

@bradwilson bradwilson closed this May 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants