Fixes #2970 InvalidCastException @ NUnit.Framework.TestFixtureSourceAttribute.BuildFrom #2971

BrunoJuchli · 2018-08-21T10:11:36Z

I've done three things:

removed the unnecessary cast from NUnit.Framework.TestFixtureSourceAttribute.BuildFrom
removed internal setter from NUnit.Framework.Internal.TestFixtureParameters.TypeArgs - as there's no usages of the setter (apart from being set by ctor)
added test cases, including one for type parameter deduction from ctor arguments (=> goal: unit tests show what's supported and what not).

Question: how can / should I run the tests included in the nunit solution locally? I've not found any documentation on this.

… arguments for [TestFixtureSource]

jnm2 · 2018-08-21T12:06:50Z

Thanks for taking the time! Getting @nunit/framework-team's attention since there isn't a discussion issue.

Question: how can / should I run the tests included in the nunit solution locally? I've not found any documentation on this.

Perhaps we should link this from the readme: https://github.com/nunit/nunit/blob/master/BUILDING.md#build-script

jnm2

LGTM besides the typo.

jnm2 · 2018-08-21T12:05:39Z

src/NUnitFramework/testdata/TestFixtureSourceData.cs

+    }
+
+    [TestFixtureSource(typeof(GenericFixtureWithTypeAndConstructorArgsSource), "Source")]
+    public class GenerixFixtureSourceWithTypeAndConstructorArgs<T>


Typo Generix

jnm2 · 2018-08-21T12:54:57Z

@BrunoJuchli I noticed that this test is failing :

NUnit.Framework.Attributes.TestFixtureSourceTests.CanRunGenericFixtureSourceWithProperTypeAndConstructorArgsProvided
>   Expected: Runnable
>   But was:  NotRunnable
>    at NUnit.Framework.Attributes.TestFixtureSourceTests.CanRunGenericFixtureSourceWithProperTypeAndConstructorArgsProvided() in C:\projects\nunit\src\NUnitFramework\tests\Attributes\TestFixtureSourceTests.cs:line 164

For suggestions on how to run tests locally other than BUILDING.md, see #2973. :D

mikkelbu · 2018-08-21T17:56:53Z

Thanks for taking the time! Getting @nunit/framework-team's attention since there isn't a discussion issue.

Isn't the (discussion) issue #2970 (or am I overlooking something)?

@BrunoJuchli I've edited your message to mention #2970, so that the issue and PR will be linked (and also such that the PR will auto close the issue when merged). Mentioning it in the titel will not work as far as I know.

mikkelbu · 2018-08-21T18:05:07Z

src/NUnitFramework/framework/Attributes/TestFixtureSourceAttribute.cs

@@ -109,7 +108,7 @@ public IEnumerable<TestSuite> BuildFrom(Type type)
        {
            Type sourceType = SourceType ?? type;

-            foreach (TestFixtureParameters parms in GetParametersFor(sourceType))
+            foreach (ITestFixtureData parms in GetParametersFor(sourceType))


Shouldn't we also remove the same unnecessary cast below? (https://github.com/nunit/nunit/pull/2971/files#diff-a7991f3c3308d3394ecebfc7b72ba252R128)

@mikkelbu
I guess that's why the test is failing. Will have a look.
Thanks!

mikkelbu · 2018-08-21T18:08:28Z

src/NUnitFramework/testdata/TestFixtureSourceData.cs

+        }
+    }
+
+    [TestFixtureSource(typeof(GenericFixtureWithTypeAndConstructorArgsSource), "Source")]


What about "Source" => nameof(GenericFixtureWithTypeAndConstructorArgsSource.Source)?

@mikkelbu
I implemented it like that at first but then I wasn't sure whether that would be supported by the compiler c# level and what the project's preference is.
All other examples in the same file use constant strings instead of the nameof(...) operator.
If nameof is fine I'll create a separate pull request to modify all string references - across the whole project - to types/members to use nameof(...) - that would make sense, right?

I guess a passing build + passing tests should be conclusive about whether nameof(...) is supported? Or does it need to be compiled on other platforms, too? (I've got windows + net)

The projects are all set to C# 6.0, so if you don't get a error squiggle, you're fine.
In typeof(...), nameof(...) cases like this with so much repetition, I'm not totally happy with any of our options :D

@jnm2 thanks!

I'd be happy to provide a PR to change string-constants to nameof(..), only adjust the newly introduced usages, or open a new issue to discuss the topic.
However, I'm not in a position to decide which is the right way to go so I'll refrain from doing anything regarding this topic until I get instructions ;-)

mikkelbu · 2018-08-21T18:09:48Z

src/NUnitFramework/testdata/TestFixtureSourceData.cs

+        };
+    }
+
+    [TestFixtureSource(typeof(GenericFixtureWithConstructorArgsSource), "Source")]


Same comment as above

jnm2 · 2018-08-21T18:22:57Z

Thanks @mikkelbu, the issue had escaped my notice.

BrunoJuchli · 2018-08-23T11:32:13Z

@jnm2 @mikkelbu
Thanks for all your feedback. I've incorporated it, apart from the nameof(...) operator (see below).
IMO it should be good to merge now.

Regarding nameof(..) I see the following options:

a) leave it as it is
b) apply nameof(...) for the whole solution (=> I'll provide a separate PR)
c) apply nameof(...) just to new code (=> I'll update this PR)

I personally would favor b) for improved refactor-safety. But I'll leave the decision to you guys, of course.
If you choose b) then you might want to create a new issue and assign it to me.

mikkelbu

LGTM.

With respect to nameof(...) then I would also say option b), but to begin with just in the test-project. @nunit/framework-team What do you think?

ChrisMaddock · 2018-08-23T21:42:35Z

Yep, I agree

jnm2 · 2018-08-24T12:21:39Z

We've already done some work towards a nameof refactor: #2623

I think we're all on board with going further. My viewpoint has been that nameof pays off most in library code and least in test code (since test code is under immediate and intense scrutiny all the time).

mikkelbu · 2018-09-11T06:39:15Z

Hi @nunit/framework-team can one of you do a second review, so that we can merge this in, as it is closing an issue which is part of the 3.11 milestone. Then we can always create a subsequent task wrt. nameof(..).

jnm2 · 2018-09-11T21:24:50Z

src/NUnitFramework/framework/Attributes/TestFixtureSourceAttribute.cs

@@ -126,7 +122,7 @@ public IEnumerable<TestSuite> BuildFrom(Type type, IPreFilter filter)
        {
            Type sourceType = SourceType ?? type;

-            foreach (TestFixtureParameters parms in GetParametersFor(sourceType))
+            foreach (ITestFixtureData parms in GetParametersFor(sourceType))


Thanks so much for doing this! This has bugged me for a while. I thought it was due to having to use the type name instead of var which silently introduces a possible invalid cast exception when the method return type is changed.

Bruno Lorenz Juchli added 4 commits August 21, 2018 11:25

made property read-only as there's no usages of the internal setter.

16d272f

fixing invalid cast exception @ TestFixtureSourceAttribute.BuildFrom

df5802e

adding explicit test for deduction of type arguments from constructor…

18d40ba

… arguments for [TestFixtureSource]

fixed tests

1e01494

BrunoJuchli changed the title ~~Fixes #2970 InvalidCastException @ NUnit.Framework.TestFixtureSourceAttribute.BuildFrom~~ [WIP] Fixes #2970 InvalidCastException @ NUnit.Framework.TestFixtureSourceAttribute.BuildFrom Aug 21, 2018

jnm2 previously approved these changes Aug 21, 2018

View reviewed changes

jnm2 added the awaiting:review label Aug 21, 2018

jnm2 added awaiting:contributor and removed awaiting:review labels Aug 21, 2018

mikkelbu reviewed Aug 21, 2018

View reviewed changes

Bruno Lorenz Juchli added 2 commits August 23, 2018 12:58

fixed typo

fe0d7cd

removed unnecessary cast and reduced code duplication

3defe5e

BrunoJuchli dismissed jnm2’s stale review via 3defe5e August 23, 2018 11:06

Merge branch 'master' into issue-2970

e1521cd

BrunoJuchli changed the title ~~[WIP] Fixes #2970 InvalidCastException @ NUnit.Framework.TestFixtureSourceAttribute.BuildFrom~~ Fixes #2970 InvalidCastException @ NUnit.Framework.TestFixtureSourceAttribute.BuildFrom Aug 23, 2018

mikkelbu approved these changes Aug 23, 2018

View reviewed changes

ChrisMaddock added awaiting:review and removed awaiting:contributor labels Aug 28, 2018

BrunoJuchli mentioned this pull request Sep 2, 2018

Support Type Arguments alongside Constructor Arguments with [TestFixtureSource] #2958

Open

jnm2 approved these changes Sep 11, 2018

View reviewed changes

jnm2 removed the awaiting:review label Sep 11, 2018

jnm2 merged commit 49ded54 into nunit:master Sep 11, 2018

Fixes #2970 InvalidCastException @ NUnit.Framework.TestFixtureSourceAttribute.BuildFrom #2971

Fixes #2970 InvalidCastException @ NUnit.Framework.TestFixtureSourceAttribute.BuildFrom #2971

Uh oh!

Conversation

BrunoJuchli commented Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnm2 commented Aug 21, 2018

Uh oh!

jnm2 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnm2 commented Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikkelbu commented Aug 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrunoJuchli Aug 21, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnm2 commented Aug 21, 2018

Uh oh!

BrunoJuchli commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikkelbu left a comment

Choose a reason for hiding this comment

Uh oh!

ChrisMaddock commented Aug 23, 2018

Uh oh!

jnm2 commented Aug 24, 2018

Uh oh!

mikkelbu commented Sep 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BrunoJuchli commented Aug 21, 2018 •

edited

Loading

jnm2 commented Aug 21, 2018 •

edited

Loading

BrunoJuchli Aug 21, 2018 •

edited

Loading

BrunoJuchli commented Aug 23, 2018 •

edited

Loading