Recognized private members in base class for Source attributes #4033

manfred-brands · 2022-01-20T14:21:34Z

mikkelbu

This of course changes the behaviour a little, but I don't think that this change will break anyone, as I think very few will have a method/field/property in the base class with the same name as the source.
@nunit/framework-team do you agree?

src/NUnitFramework/framework/Attributes/TypeGetMemberExtension.cs

stevenaw · 2022-02-05T18:50:05Z

This of course changes the behaviour a little, but I don't think that this change will break anyone, as I think very few will have a method/field/property in the base class with the same name as the source. @nunit/framework-team do you agree?

I'm a little hesitant about this myself. While it seems like a simple fix to look for private members, I'm concerned it may also introduce a lot of unexpected edge cases. For example, what if a base class and a child class each define identically named methods with identical arguments? Non-private members would have this caught by the C# compiler as an error but the guard rails are off if we try to go around that.

I'm a little rusty on how/if private members can be used in other inheritance situations (like a virtual test method which gets overridden by a child class) but I see there's a bit of a discussion in the originating issue that this works with protected or public members and that perhaps an analyzer could help guide around this. I'm wondering if a in-framework fix could instead provide a more helpful error message.

Co-authored-by: Mikkel Nylander Bundgaard <mikkelbu@users.noreply.github.com>

manfred-brands · 2022-02-06T03:45:08Z

For example, what if a base class and a child class each define identically named methods with identical arguments?

In the previous case, the GetMember would have passed the child class parameters to both the child and the base class and never the base class parameter to the base class test. Most likely causing the base class test to fail.
So we have no real 'hiding' of naming in base classes. It is more like the child class can 'override' a static member.

This of course changes the behaviour a little, but I don't think that this change will break anyone, as I think very few will have a method/field/property in the base class with the same name as the source.

This is actually used in various NUnit Framework test where the base class (ConstraintTestBase) depends on a SuccessData source which it doesn't define itself. The child class (EmptyConstraintTest) defines this, but the compiler complains it isn't used.
Looks like a candidate for abstract static members

In the changes for this PR it would test nothing :-( as GetMemberIncludingFromBase returns 2 members and that is translated to null and silently ignored! Personally I would prefer an error if the requested data source cannot be (uniquely) found.

To return previous behaviour, I will update the GetMemberIncludingFromBase method to bail if it finds a member. So that if the member is defined in the child class it will use that regardless of what is defined in the base class.

stevenaw · 2022-02-07T18:33:41Z

This is actually used in various NUnit Framework test where the base class (ConstraintTestBase) depends on a SuccessData source which it doesn't define itself.

Hm, you are quite right, I hadn't thought about it this way. I suppose this is mostly to facilitate using nameof(), which those tests predate.

It still "feels" more right to me to expect a member which is declared in a base class and potentially defined or overridden in a child class to require "protected" or "public" visibility much the way the C# compiler enforces member visibility, but I can also see it being a bit awkward when the resolution (via nameof()) and invocation (via the test runner) don't both occur at compile-time.

I still think we may want to improve error messages to guide a user to use protected visibility over private when using nameof() in an abstract class, but perhaps that can just be done via an analyzer (I think @mikkelbu has already made the corresponding GH issue). Automagically resolving private members here definitely seems like a nice usability feature.

Thanks @manfred-brands (and thanks for the latest changes to help ensure this is non-breaking). Things are a bit busy for me, but I'll try and do a proper review over the next few days

manfred-brands · 2022-02-08T00:40:29Z

It still "feels" more right to me to expect a member which is declared in a base class and potentially defined or overridden in a child class to require "protected" or "public" visibility.

There are two ways of 'overriding' a static member.

If the member is private, the derived class can have a member with the same name without getting any warnings.
If the member is protected, the derived class must use the new keyword to replace it.

The initial ticket has both the attribute and the member applied to the base class only. The fact is hidden from the child class which doesn't need to know. Therefore private accessibility sounds right. But NUnit operates on the child class and will find the member in the derived class first which might not be what is wanted.

Thinking more about this, I now tend to agree with you. If the member would be protected then a developer making a child class would get a warning if it accidentally uses the same name for a member. If deliberate, they can use the new keyword to state that intention. We do need to verify the latter works.

I still think we may want to improve error messages to guide a user to use protected visibility over private when using nameof() in an abstract class.

I agree that we would need error messages, both analyzer and runtime, if the child class has a member with the same name as a private member in a base class if such a member is used in any of the Source attributes.

If nunit itself would use nunit.analyzer (which has been discussed), then they would have a warning in ConstraintTestBase that the "SuccessData" string passed to the TestCaseSource doesn't exist, unless we don't warn on abstract classes and only check the child which would be ideal as we otherwise have no way to enforce the child class defining the member.

What is the underlying reason for the if (members.Length == 1) tests in the ...SourceAttribute classes. We neither get an error if the member is not defined nor if it defined twice. Both in my opinion are errors.

stevenaw · 2022-02-08T01:24:20Z

Ah, yes, you're quite right. Somehow I had missed the "static" part of this scenario. You're absolutely right. Sorry about that @manfred-brands

manfred-brands · 2022-02-08T09:10:15Z

@stevenaw I was not shouting, I thought I was numbering, but they became big bold headings. :-(

stevenaw · 2022-02-08T18:40:31Z

Not to worry, no offense taken at all @manfred-brands . I hadn't realized when making my earlier comments that we had also been considering static overriding, but of course your suggestions make sense in that context, and I am glad the conversation was steered back in that direction 🙂

stevenaw · 2022-02-10T16:13:13Z

src/NUnitFramework/tests/Attributes/TestFixtureSourceTests.cs

@@ -44,9 +44,12 @@ public class TestFixtureSourceTests
        [TestCase(typeof(SourceReturnsFixtureParameters))]
        [TestCase(typeof(ExtraTestFixtureAttributeIsIgnored))]
        [TestCase(typeof(TestFixtureMayUseMultipleSourceAttributes))]
+        [TestCase(typeof(DerivedClassUsingBaseClassDataSource))]
+        [TestCase(typeof(BaseClassUsingDerivedClassDataSource))]


👍 Thanks for the thorough test cases!

Test Driven Development. First create a test case that shows the problem, then fix it.

stevenaw

Thanks @manfred-brands ! Everything looks great. I left a few suggestions for a few existing classes or extensions which could help what you're trying to do here.

stevenaw · 2022-02-10T16:16:34Z

src/NUnitFramework/framework/Attributes/TypeGetMemberExtension.cs

+
+namespace NUnit.Framework
+{
+    internal static class TypeGetMemberExtension


Thanks for making these extensions internal visibility by default.

Suggestion: I think there may be an existing + recently created TypeExtensions class which lives in Internal/Extensions. Would it make sense for this new method to live there too?

I had looked for extension classes, but missed this one. Method moved and Tests added.

stevenaw · 2022-02-10T16:26:23Z

src/NUnitFramework/framework/Attributes/TypeGetMemberExtension.cs

+            {
+                members.AddRange(type.GetMember(name, flags));
+            }
+            while (members.Count == 0 && (type = type.BaseType) != null);


Suggestion: NUnit.Framework.Internal.Reflect also defines a few extension methods over the Type type, including this "walking the class hierarchy" behaviour. Would it make sense to use TypeAndBaseTypes here instead?

I know this part changed recently, but if I'm reading the current version of the code right, we also only walk the hierarchy until the first time that GetMember returns something. Is the List<> still required? Coupled with the above suggestion, would it be simpler to change this to a simple foreach + early return?

That class is an even better location, moved it and the tests again. Then there also is the class Extensions.

You are absolutely right that the List<> is no longer required. Factored out.

stevenaw · 2022-02-10T16:27:31Z

src/NUnitFramework/framework/Attributes/TestFixtureSourceAttribute.cs

@@ -198,8 +198,8 @@ public IEnumerable<ITestFixtureData> GetParametersFor(Type sourceType)
            if (SourceName == null)
                return Reflect.Construct(sourceType) as IEnumerable;

-            MemberInfo[] members = sourceType.GetMember(SourceName,
-                    BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Static | BindingFlags.Instance | BindingFlags.FlattenHierarchy);


Thanks for catching this. It's a little awkward for our purposes that FlattenHierarchy doesn't handle private members

Factored out the now unnecessary List<>.

stevenaw · 2022-02-13T01:19:16Z

LGTM! Thanks for your contribution @manfred-brands 👍

Recognized private members in base class for Source attributes

a6f9a12

mikkelbu approved these changes Feb 3, 2022

View reviewed changes

src/NUnitFramework/framework/Attributes/TypeGetMemberExtension.cs Outdated Show resolved Hide resolved

src/NUnitFramework/framework/Attributes/TypeGetMemberExtension.cs Outdated Show resolved Hide resolved

Apply suggestions from code review

08ce83b

Co-authored-by: Mikkel Nylander Bundgaard <mikkelbu@users.noreply.github.com>

Restore behaviour were derived class 'overrides' data source.

dc70973

stevenaw reviewed Feb 10, 2022

View reviewed changes

stevenaw requested changes Feb 10, 2022

View reviewed changes

Moved GetMemberIncludingFromBase method and added nunit test.

d1ee5ae

Factored out the now unnecessary List<>.

stevenaw approved these changes Feb 13, 2022

View reviewed changes

stevenaw merged commit de1fe90 into nunit:v3.13-dev Feb 13, 2022

stevenaw added this to the 3.13.3 milestone Feb 13, 2022

manfred-brands mentioned this pull request Feb 16, 2022

Master issue/4032 test fixture source #4039

Merged

manfred-brands deleted the issue/4032_TestFixtureSource branch February 16, 2022 23:26

Recognized private members in base class for Source attributes #4033

Recognized private members in base class for Source attributes #4033

Uh oh!

Conversation

manfred-brands commented Jan 20, 2022

Uh oh!

mikkelbu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stevenaw commented Feb 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

manfred-brands commented Feb 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevenaw commented Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

manfred-brands commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevenaw commented Feb 8, 2022

Uh oh!

manfred-brands commented Feb 8, 2022

Uh oh!

stevenaw commented Feb 8, 2022

Uh oh!

stevenaw Feb 10, 2022

Choose a reason for hiding this comment

Uh oh!

manfred-brands Feb 11, 2022

Choose a reason for hiding this comment

Uh oh!

stevenaw left a comment

Choose a reason for hiding this comment

Uh oh!

stevenaw Feb 10, 2022

Choose a reason for hiding this comment

Uh oh!

manfred-brands Feb 11, 2022

Choose a reason for hiding this comment

Uh oh!

stevenaw Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manfred-brands Feb 11, 2022

Choose a reason for hiding this comment

Uh oh!

stevenaw Feb 10, 2022

Choose a reason for hiding this comment

Uh oh!

stevenaw commented Feb 13, 2022

Uh oh!

Uh oh!

stevenaw commented Feb 5, 2022 •

edited

Loading

manfred-brands commented Feb 6, 2022 •

edited

Loading

stevenaw commented Feb 7, 2022 •

edited

Loading

manfred-brands commented Feb 8, 2022 •

edited

Loading

stevenaw Feb 10, 2022 •

edited

Loading