3053 - Conversion from TestCase string parameter to DateTimeOffset by stevenaw · Pull Request #3098 · nunit/nunit

stevenaw · 2018-11-26T19:56:27Z

Updates TestCaseAttribute to convert String parameter to DateTimeOffset
I noticed the existing type conversion logic wasn't applied to params arrays, so I expanded it to work there too (I added a test for the existing TimeSpan logic for that too).

Added Dependencies

NET Standard 1.4

System.ComponentModel.TypeConverter

Scope Creep

I also noticed that the ValuesAttribute handles most type conversions the TestCaseAttribute does, so I added this string -> DateTimeOffset conversion for it too. I also consolidated some of the type conversion logic between them (TestCaseAttribute now uses ParamAttributeTypeConversions too). In the process, I had to update ParamAttributeTypeConversions to map values to nullable primitives, it looks like previously we only mapped null to nullable primitives.

One potential change to existing behaviour is that TestCaseAttribute previously called TimeSpan.Parse() without providing a culture, whereas it looked like most other conversions used CultureInfo.InvariantCulture (including when ValuesAttribute converts a string to a TimeSpan. So in the process of centralizing the type conversion logic, TestCaseAttribute now also uses CultureInfo.InvariantCulture when converting a string to a TimeSpan.

In keeping with comment #3053 (comment), TimeSpan now also gets converted using TypeConverter instead of Convert, as it does not implemement IConvertible.

Since this is a bit more than just "Conversion from TestCase string parameter to DateTimeOffset" covered in this PR, let me know if a second GH issue and/or PR should be required for the broader changes.

stevenaw · 2018-11-26T21:16:07Z

+        /// <returns>
+        /// <c>true</c> if <paramref name="value"/> was converted and <paramref name="convertedValue"/> should be used;
+        /// <c>false</c> is no conversion was applied and <paramref name="convertedValue"/> should be ignored
+        /// </returns>


This xmldoc comment was copied over from TestCaseAttribute's "PerformSpecialConversion"
https://github.com/nunit/nunit/pull/3098/files#diff-012b7fc50160d70b26586addebf7ce1fL395

ChrisMaddock

Thanks for putting this together @stevenaw - sorry it's taken a while to review, it's not a nice part of the codebase to look at! 😅 Left a few comments - this will also need a second review from another member of @nunit/framework-team. 🙂

ChrisMaddock · 2018-12-30T14:32:47Z

-        {
-            argAsTargetType = null;
-            if (arg == null)
-                return false;


Note for other reviewers: the change in logic took me a bit of time to get my head around here - previously this would always return false for null - it will now return true if the arg is null AND the type is nullable. I can't see anywhere this would cause a problem.

ChrisMaddock · 2018-12-30T14:38:42Z

-            // Convert.ChangeType doesn't work for TimeSpan from string
-            if ((targetType == typeof(TimeSpan) || targetType == typeof(TimeSpan?)) && arg is string)
-            {
-                argAsTargetType = TimeSpan.Parse((string)arg);


Re: Your comment about CultureInvariant - good call. I'd consider this a 'bug' - hopefully not too many people will have come to rely on the behaviour! We discussed this in more detail a while back here: #2008

ChrisMaddock · 2018-12-30T14:43:41Z

        }

        [Test]
-        public void CanConvertIntToDecimal([Values(12)]decimal x)


Note to future reviewers: These three 'removed' tests had just been combined into a single test on line 94. Seems fine to me. 👍

stevenaw · 2018-12-30T16:03:54Z

Thanks for the review @ChrisMaddock !
I gave a quick reply to a question, but it might be a few days until I can parse and respond to the rest of the review. Thanks for pointing out the nuspec thing though!

ChrisMaddock · 2018-12-30T17:01:57Z

Don’t worry - it’s taken me over a month to get round to looking at this! 😅 enjoy New Years!

stevenaw · 2019-02-11T12:15:45Z

Thanks for the feedback @jnm2 !
I'm on vacation right now, but should be able to get to this within a week or two

Add tests Add TypeConverter for netstandard 1.4

…e from int support to RangeAttribute

stevenaw · 2019-02-18T22:08:17Z


        // See XML docs for the ParamAttributeTypeConversions class.
-        private static readonly Type[] Int32RangeConvertibleToParameterTypes = { typeof(int), typeof(sbyte), typeof(byte), typeof(short), typeof(decimal) };
+        private static readonly Type[] Int32RangeConvertibleToParameterTypes = { typeof(int), typeof(sbyte), typeof(byte), typeof(short), typeof(decimal), typeof(long), typeof(double) };


NOTE: this change is required based on this line: https://github.com/nunit/nunit/pull/3098/files#diff-64bcafa7925c912b792f366a7971facfR121

This was a hidden inconsistency between our attributes. TestCaseAttribute allowed int -> long and int -> double but RangeAttribute did not explicitly support the conversion. Now they are allowed on both (in nullable and non-nullable forms)

This is good, we want this. The idea behind this list of special-cased conversions is to emulate C#'s implicit casts.

stevenaw · 2019-03-21T00:49:45Z

Thanks for taking the time to review this earlier @jnm2
I'm unable to update the labels on this PR, but if you have time to re-review, I think I've incorporated all your suggestions.

jnm2 · 2019-03-21T00:54:18Z


        // See XML docs for the ParamAttributeTypeConversions class.
-        private static readonly Type[] Int32RangeConvertibleToParameterTypes = { typeof(int), typeof(sbyte), typeof(byte), typeof(short), typeof(decimal) };
+        private static readonly Type[] Int32RangeConvertibleToParameterTypes = { typeof(int), typeof(sbyte), typeof(byte), typeof(short), typeof(decimal), typeof(long), typeof(double) };


This is good, we want this. The idea behind this list of special-cased conversions is to emulate C#'s implicit casts.

jnm2 · 2019-04-24T01:01:55Z

@rprouse Okay to merge for 3.12?

mikkelbu · 2019-04-25T20:37:52Z

I'll probably have time to look at this during the weekend.

rprouse

Thanks for doing all this. I apologize that it took so long to review and merge, everyone on the team has been pretty busy with work lately so we haven't been doing as good a job reviewing as we should. Thanks for being patient.

stevenaw · 2019-04-28T17:50:54Z

Thanks!

stevenaw commented Nov 26, 2018

View reviewed changes

Comment thread src/NUnitFramework/framework/Internal/ParamAttributeTypeConversions.cs

stevenaw commented Nov 26, 2018

View reviewed changes

mikkelbu added the awaiting:review label Nov 27, 2018

ChrisMaddock reviewed Dec 30, 2018

View reviewed changes

jnm2 requested changes Feb 11, 2019

View reviewed changes

Comment thread src/NUnitFramework/framework/Internal/ParamAttributeTypeConversions.cs Outdated

Comment thread src/NUnitFramework/framework/Internal/ParamAttributeTypeConversions.cs

Comment thread src/NUnitFramework/framework/Internal/ParamAttributeTypeConversions.cs Outdated

jnm2 added awaiting:contributor and removed awaiting:review labels Feb 11, 2019

stevenaw added 11 commits February 18, 2019 10:09

Use TypeConverter when can't use Convert

8ffc376

Add tests Add TypeConverter for netstandard 1.4

Update test case attributes

efe9765

DateTimeOffset as a Values[]

37ba29a

Use ParamAttributeTypeConversions from TestCaseAttribute

a39e3a1

Add a few tests for implicitly-allowed conversions

53e273d

Namespace cleanup

d351beb

Remove array conversions

25a8188

Add explicit assertions to valuesattribute tests

57df4ed

Add nuspec reference

161b7f9

Nullable.GetUnderlyingValue()

e543ce8

Make internal nullable checks consistent - but also adds long + doubl…

f4f22f3

…e from int support to RangeAttribute

stevenaw force-pushed the 3053/datetimeoffset branch from 4d4ff77 to f4f22f3 Compare February 18, 2019 21:59

stevenaw commented Feb 18, 2019

View reviewed changes

Comment thread src/NUnitFramework/framework/Internal/ParamAttributeTypeConversions.cs

stevenaw commented Feb 18, 2019

View reviewed changes

jnm2 added awaiting:review and removed awaiting:contributor labels Mar 21, 2019

jnm2 previously approved these changes Mar 21, 2019

View reviewed changes

jnm2 requested a review from a team March 21, 2019 00:54

Remove whitespace

1abd5cf

stevenaw dismissed jnm2’s stale review via 1abd5cf March 21, 2019 23:32

jnm2 approved these changes Mar 22, 2019

View reviewed changes

jnm2 requested a review from a team March 22, 2019 00:03

jnm2 mentioned this pull request Apr 24, 2019

Assert.Ignore breaks when a Task is returned w/o using async/await #3138

Closed

stevenaw mentioned this pull request Apr 27, 2019

Consolidate type conversion code #3219

Open

rprouse approved these changes Apr 28, 2019

View reviewed changes

rprouse merged commit 580a1ab into nunit:master Apr 28, 2019

Conversation

stevenaw commented Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Added Dependencies

NET Standard 1.4

Scope Creep

Uh oh!

Uh oh!

stevenaw Nov 26, 2018

Choose a reason for hiding this comment

Uh oh!

ChrisMaddock left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ChrisMaddock Dec 30, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ChrisMaddock Dec 30, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ChrisMaddock Dec 30, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stevenaw commented Dec 30, 2018

Uh oh!

ChrisMaddock commented Dec 30, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevenaw commented Feb 11, 2019

Uh oh!

Uh oh!

stevenaw Feb 18, 2019

Choose a reason for hiding this comment

Uh oh!

jnm2 Mar 21, 2019

Choose a reason for hiding this comment

Uh oh!

stevenaw commented Mar 21, 2019

Uh oh!

Uh oh!

jnm2 Mar 21, 2019

Choose a reason for hiding this comment

Uh oh!

jnm2 commented Apr 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikkelbu commented Apr 25, 2019

Uh oh!

rprouse left a comment

Choose a reason for hiding this comment

Uh oh!

stevenaw commented Apr 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

stevenaw commented Nov 26, 2018 •

edited

Loading

jnm2 commented Apr 24, 2019 •

edited

Loading