Fixed Join_ObjectArray test in StringTests by AlexRadch · Pull Request #14083 · dotnet/corefx

AlexRadch · 2016-11-29T08:29:09Z

Join issue for first null in object array is fixed. So calling string.Join(",", null, 1, 2, 3) now return ",1,2,3", but not empty string as before. See dotnet/coreclr#8114

AlexRadch · 2016-11-29T08:39:00Z

Fixed test pass for local run in src\System.Runtime\tests\ folder by executing msbuild /t:BuildAndTest command.
But test fail in root folder if run build command. I do not know how to fix that.
I tried to use netcoreapp11 instead netstandard17 but then test fail in both cases.

hughbe · 2016-11-29T12:57:30Z

-                Assert.Equal(expected, string.Join(separator, (IEnumerable<object>)values));
-            }
+            var exp = expected;
+            if (values.Length > 0 && values[0] == null) // Join return empty string if array[0] is null


nit: put this inside an #else condition after the block of #if netstandard17

@hughbe but early at here #13747 (comment) @danmosemsft say to remove #else and I changed code to more complicated to remove #else. Can I use #else or can't?

Ah okay, I'm not bothered. It seems that the #else would make more sense, as you're overwriting the value immediately and that code is dead before netstandard17

hughbe · 2016-11-29T12:57:47Z

-            {
-                Assert.Equal(expected, string.Join(separator, (IEnumerable<object>)values));
-            }
+            var exp = expected;


Can we make this name more descriptive?

I changed name

hughbe · 2016-11-29T12:57:56Z

@@ -1619,22 +1619,25 @@ public static IEnumerable<object[]> Join_ObjectArray_TestData()
            yield return new object[] { "$$", new object[] { "Foo", null, "Baz" }, "Foo$$$$Baz" };

            // Join does nothing if array[0] is null


Stale comment?

Oh, sorry, I will change it.

stephentoub · 2016-11-29T14:10:41Z

-            // Join does nothing if array[0] is null
-            yield return new object[] { "$$", new object[] { null, "Bar", "Baz" }, "" };
+            // Test join when first value is null
+            yield return new object[] { "$$", new object[] { null, "Bar", "Baz" }, "$$Bar$$Baz" };


This will end up failing when run on desktop, right? We'll need to separate it out into a separate test that's disabled for NetFramework.

I think it pass test on NetFramework because I wrote next code

#if !netstandard17 if (values.Length > 0 && values[0] == null) // Join return nothing when first value is null expected = ""; #endif

stephentoub · 2016-11-29T14:11:11Z

        [MemberData(nameof(Join_ObjectArray_TestData))]
        public static void Join_ObjectArray(string separator, object[] values, string expected)
        {
+            var enumerableExpected = expected;


Nit: var => string

karelz · 2016-11-29T16:38:42Z

@AlexRadch please put important relevant info in the bug/PR description:

Join issue for first null in object array is fixed. So calling string.Join(",", null, 1, 2, 3) now return ",1,2,3", but not empty string as before. See dotnet/coreclr#8114.

AlexRadch · 2016-11-29T17:17:54Z

@AlexRadch please put important relevant info in the bug/PR description:

@karelz sorry I am beginning in english. Can you help with relevant description to me?

karelz · 2016-11-29T17:33:00Z

@karelz sorry I am beginning in english. Can you help with relevant description to me?

Sure and no worries, I have been there myself some time ago :)
BTW: The description text I meant was already in my reply + I already updated the top text in the PR.

The relevant information is usually links to bugs and some context, description like the one you had in previous PR.
In this PR you had just "We fixed string.Join, now add test" which does not describe which fix of string.Join you mean. Second question that immediately pops is "why tests were not part of the fix" ... both of these are naturally answered by the text in your previous PR which links the string.Join bug fix (in coreclr) and says briefly what it was about.

AlexGhiondea · 2016-11-29T18:17:46Z

-        [ActiveIssue(13747)]
        [Theory]
        [MemberData(nameof(Join_ObjectArray_TestData))]
        public static void Join_ObjectArray(string separator, object[] values, string expected)


I think the right approach is to split this in 2 methods.

The first method will test the new behavior and you will have to add these 2 attributes to make sure it does not run on the previous versions of the platform:

[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)] [SkipOnTargetFramework(TargetFrameworkMonikers.Netcoreapp1_0)]

And another method will test the old behavior. For that you will have to add this attribute:

[SkipOnTargetFramework(TargetFrameworkMonikers.Netcoreapp1_1)]

To make sure it is not running on the new version of the platform.

As I mentioned in the other PR, the netstandard17 define is actually set for more build configuration that you need it.

@AlexGhiondea I splited test in two

AlexGhiondea · 2016-11-29T19:44:08Z

+        public static void Join_ObjectArray_WithNullIssue(string separator, object[] values, string expected)
+        {
+            string enumerableExpected = expected;
+            if (values.Length > 0 && values[0] == null) // Join return nothing when first value is null


Thanks for splitting this out.

Given that you have 2 methods now you can remove this special case for the null value and have 2 data generator methods for the two tests.

The idea is that you can then remove the #if-def from the test data generator.

Do you suggest split test data too?

Yes -- I think it would simplify the tests since they don't have to account for unexpected values.

Join_ObjectArray_WithNullIssue test tests two methods string.Join(separator, values) and string.Join(separator, (IEnumerable<object>)values)). First method has issue but second hasn't.

So to test both we should split data in 2 and split test in 3 or 4 not in 2 as now.
So at begin we have 1 data and 1 test method with one #if and one if inside.
To remove #if I splited test in 2 with common data. But why I should split data in 2 and test in 3 or 4 to remove one if.

Original test that I am trying to fix for netcoreapp11 has if inside, But nobody did not split it on two with two data.
I think if we split one data and two tests in 2 data and 3 or 4 tests is not simplifying it is unnecessary complication.

What I was trying to avoid is doing both the attribute on the methods to skip them on various platforms and having an #if/def inside the tests.

I believe the reason why it was not split before is because we were using an if and not an #if/def.

Anyway, at this point let's just leave them as they are but please add a comment explaining why we have the if that tests for null inside the tests.

Is next not enough?
if (values.Length > 0 && values[0] == null) // Join return nothing when first value is null
It is old comment that was before. I should extend it?

stephentoub · 2016-11-30T19:41:27Z

@dotnet-bot test this please

AlexGhiondea · 2016-12-01T03:38:13Z

@dotnet-bot test this please

AlexGhiondea · 2016-12-01T05:47:38Z

Thank you for your contribution @AlexRadch !

karelz · 2016-12-01T11:34:05Z

ditto, thanks for your contribution, we appreciate it!

jayaranigarg · 2017-11-02T10:43:08Z

Is this ported to netfx yet?

* Fixed Join_ObjectArray test in StringTests Commit migrated from dotnet/corefx@57eb42f

Fixed Join_ObjectArray test in StringTests

24c420a

dnfclas added the cla-already-signed label Nov 29, 2016

AlexRadch mentioned this pull request Nov 29, 2016

Join(string separator, params object[] values) is fixed for first null in object array #13747

Closed

hughbe approved these changes Nov 29, 2016

View reviewed changes

enumerableExpected

4bff52e

stephentoub reviewed Nov 29, 2016

View reviewed changes

var => string

9ae1b2d

karelz added the area-System.Runtime label Nov 29, 2016

karelz added the netfx-port-consider label Nov 29, 2016

karelz assigned AlexRadch, AlexGhiondea and joperezr Nov 29, 2016

AlexGhiondea suggested changes Nov 29, 2016

View reviewed changes

I splited test on two

016bf68

AlexGhiondea reviewed Nov 29, 2016

View reviewed changes

AlexGhiondea approved these changes Dec 1, 2016

View reviewed changes

AlexGhiondea merged commit 57eb42f into dotnet:master Dec 1, 2016

AlexRadch deleted the String-Join_ObjectArray-Test branch December 1, 2016 06:15

karelz modified the milestone: 1.2.0 Dec 3, 2016

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Fixed Join_ObjectArray test in StringTests (dotnet/corefx#14083)

71be9fe

* Fixed Join_ObjectArray test in StringTests Commit migrated from dotnet/corefx@57eb42f

		@@ -1619,22 +1619,25 @@ public static IEnumerable<object[]> Join_ObjectArray_TestData()
		yield return new object[] { "$$", new object[] { "Foo", null, "Baz" }, "Foo$$$$Baz" };

		// Join does nothing if array[0] is null

Conversation

AlexRadch commented Nov 29, 2016 • edited by karelz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexRadch commented Nov 29, 2016

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!

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!

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!

karelz commented Nov 29, 2016

Uh oh!

AlexRadch commented Nov 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karelz commented Nov 29, 2016

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!

Choose a reason for hiding this comment

Uh oh!

AlexRadch Nov 30, 2016 • 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!

AlexRadch Dec 1, 2016 • 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!

stephentoub commented Nov 30, 2016

Uh oh!

AlexGhiondea commented Dec 1, 2016

Uh oh!

AlexGhiondea commented Dec 1, 2016

Uh oh!

karelz commented Dec 1, 2016

Uh oh!

jayaranigarg commented Nov 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

AlexRadch commented Nov 29, 2016 •

edited by karelz

Loading

AlexRadch commented Nov 29, 2016 •

edited

Loading

AlexRadch Nov 30, 2016 •

edited

Loading

AlexRadch Dec 1, 2016 •

edited

Loading