Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fixed Join_ObjectArray test in StringTests #14083

Merged
AlexGhiondea merged 4 commits into from
Dec 1, 2016
Merged

Fixed Join_ObjectArray test in StringTests #14083

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
24c420a
Fixed Join_ObjectArray test in StringTests
AlexRadch Nov 29, 2016
4bff52e
enumerableExpected
AlexRadch Nov 29, 2016
9ae1b2d
var => string
AlexRadch Nov 29, 2016
016bf68
I splited test on two
AlexRadch Nov 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions src/System.Runtime/tests/System/StringTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1618,23 +1618,32 @@ public static IEnumerable<object[]> Join_ObjectArray_TestData()
yield return new object[] { null, new object[] { "Foo", "Bar", "Baz" }, "FooBarBaz" };
yield return new object[] { "$$", new object[] { "Foo", null, "Baz" }, "Foo$$$$Baz" };

// 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" };
Copy link
Copy Markdown
Member

@stephentoub stephentoub Nov 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

@AlexRadch AlexRadch Nov 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Join should ignore objects that have a null ToString() value
yield return new object[] { "|", new object[] { new ObjectWithNullToString(), "Foo", new ObjectWithNullToString(), "Bar", new ObjectWithNullToString() }, "|Foo||Bar|" };
}

[ActiveIssue(13747)]
[Theory]
[MemberData(nameof(Join_ObjectArray_TestData))]
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework | TargetFrameworkMonikers.NetcoreUwp | TargetFrameworkMonikers.Netcoreapp1_0)]
public static void Join_ObjectArray(string separator, object[] values, string expected)
Copy link
Copy Markdown
Contributor

@AlexGhiondea AlexGhiondea Nov 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

@AlexRadch AlexRadch Nov 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlexGhiondea I splited test in two

{
Assert.Equal(expected, string.Join(separator, values));
if (!(values.Length > 0 && values[0] == null))
{
Assert.Equal(expected, string.Join(separator, (IEnumerable<object>)values));
}
Assert.Equal(expected, string.Join(separator, (IEnumerable<object>)values));
}

[Theory]
[MemberData(nameof(Join_ObjectArray_TestData))]
[SkipOnTargetFramework(TargetFrameworkMonikers.Netcoreapp1_1)]
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
Copy link
Copy Markdown
Contributor

@AlexGhiondea AlexGhiondea Nov 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

@AlexRadch AlexRadch Nov 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you suggest split test data too?

Copy link
Copy Markdown
Contributor

@AlexGhiondea AlexGhiondea Nov 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

@AlexRadch AlexRadch Nov 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@AlexGhiondea AlexGhiondea Dec 1, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

@AlexRadch AlexRadch Dec 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

expected = "";
Assert.Equal(expected, string.Join(separator, values));
Assert.Equal(enumerableExpected, string.Join(separator, (IEnumerable<object>)values));
}

[Fact]
Expand Down