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

[AlexRadch]

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

[AlexGhiondea]

@AlexRadch thanks for updating the tests. Since the CoreCLR that has your change is not in use in CoreFx (yet) the tests are going to fail.

We might need to temporarily disable them so that the CI for the new CoreCLR can be ingested (/cc @joperezr @stephentoub).

I suggest we disable them for now until we can get the updated CoreCLR checked in. We can then re-enable them and update the expected behavior.

[stephentoub]

We might need to temporarily disable them

I've already done so here:
#13742

Since the CoreCLR that has your change is not in use in CoreFx (yet) the tests are going to fail.

Note that the tests will also fail even after the update when they run against desktop or netcoreapp1.0, etc. The tests will need to be specialized some way to deal with that.

[AlexRadch]

@stephentoub I do not know how to specialize tests for different netStandard versions. There is any instruction or example?

[stephentoub]

I do not know how to specialize tests for different netStandard versions

@AlexGhiondea, can you assist? I was speaking about this earlier with @weshaggard, seems we don't have a great approach right now. Seems like we might use the SkipOnTargetFramework attribute on two different tests, or we might have two different tests each conditionally included based on build flags, etc.

[AlexGhiondea]

@AlexRadch it sounds like the simplest way to fix this (while it is still unfortunate) is to have 4 tests total. 2 that run on desktop/netcoreapp1.0 and 2 that run on netcoreapp1.1.

Something like this for the new behavior of the tests

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

And like this on the old behavior of the tests.

[SkipOnTargetFramework(TargetFrameworkMonikers.Netcoreapp1_1)]

@weshaggard please chime in if there is a better way to do this.

[AlexRadch]

@AlexGhiondea I added conditional compilation for testing on netstandard17 or early versions.

[AlexGhiondea]

@AlexRadch thanks for updating this. However, I am not sure if this will actually work. We are setting that define for all projects when '$(TargetGroup)'==''.

And in the builds file we actually explicitly pass differenty TargetGroup values for testing:
https://github.com/dotnet/corefx/blob/master/src/System.Runtime/tests/System.Runtime.Tests.builds#L11

The other option would be to split the test into 2 different files and include the files as needed. There is already precedend for doing that here:
https://github.com/dotnet/corefx/blob/master/src/System.Runtime/tests/System.Runtime.Tests.csproj#L110

[weshaggard]

Yes we should definitely not use netstandard17 as the pivot because that is saying that all netstandard17 plaforms have this new behavior. We should either do what @AlexGhiondea suggested with skip or use netcoreapp11 as your define and only test for the new behavior in that build configuration. I kind of like the build configuration a little better personally but I don't have strong opinion either way.

[AlexRadch]

@weshaggard all platforms coped join issue from .Net Framework. Now we fixed join issue in netCore1.1.
And now netCore1.1 implements netStandard1.7.
When other platform will implement netStandard1.7 this Join test will fail so they will have choice:

1. Fix join issue in platform
2. Change this join test for specific platform

Now we do not know what they will choice, so I think use netstandard17 more good than use netcoreapp11.
What are you think about this?

[AlexGhiondea]

@AlexRadch have you tried running your change against netcoreapp1.0? You can do that by passing /p:FilterToTestTFM=netcoreapp1.1 when building the .builds project for the tests.

[joperezr]

You can do that by passing /p:FilterToTestTFM=netcoreapp1.1 when building the .builds project for the tests.

Just correcting, in order to test in netcoreapp1.0 you have to run:

msbuild src\System.Runtime\tests\System.Runtime.Tests.builds /p:FilterToTestTFM=netcoreapp1.0

Also, note that this run will most likely fail with a bunch of TypeLoadException. The reason for this is https://github.com/dotnet/coreclr/issues/7607

[danmoseley]

@AlexRadch the CLR updated

[danmoseley]

@AlexGhiondea it's sufficient for @AlexRadch to change the define from netstandard17 to netcoreapp11 and delete the #else, if I understand right? Then he should do msbuild src\System.Runtime\tests\System.Runtime.Tests.builds and check there's no errors? And we can merge this?

I want to clear this old PR out.

[AlexGhiondea]

@danmosemsft that should do it, yes.

[AlexRadch]

@danmosemsft I tried change netstandard17 to netcoreapp11 but it does not works. I did not find any test that uses netcoreapp11 but there are many tests that used netstandard17.
See next:

src\System.Runtime\tests\System\IO\FileLoadExceptionTests.cs 
src\System.Runtime\tests\System\IO\FileNotFoundExceptionTests.cs
src\System.Runtime\tests\System\MissingFieldExceptionTests.cs
src\System.Runtime\tests\System\MissingMethodExceptionTests.cs

[AlexRadch]

I recreated this PR #14083

[danmoseley]

@AlexRadch I don't mind making a new PR, but any particular reason?

[karelz]

Moving the tag netfx-port-consider to the new PR (#14083).

[AlexRadch]

@AlexRadch I don't mind making a new PR, but any particular reason?

@danmosemsft I am begging in github and my PRs have many strange commits. All wrote to me to remove these strange commits. I tried different ways but I could not remove them. Finally I removed repository fork. So now I can not make commits to this PR and I created new PR to commit code changes.

[karelz]

I am beggining in github and my PRs have many strange commits

I think the root cause was that you don't use git "correctly". I am git beginner myself, but the key thing, as I understand it, is to not merge during PR, just keep your branch stale - it will be merged after all reviews.

See our contributions docs - the second to last bullet.

[danmoseley]

Right - we've all been there (with Git - I come from Perforce and VSTS myself). Generally what you do is just keep committing to your branch and pushing, then when someone merges your PR, we will hit "Squash" and it will look like a single commit.

If Github shows that you must resolve conflicts before merging, the right procedure is something like

git checkout master
git pull
git checkout <PR branch>
git rebase master
<resolve any conflicts, and git-add the files>
git commit
git push --force origin <PR branch>

You still stay in the same PR.

One good reason to make a new PR is because the conversation has got very confusing and off track. That is rare.

[AlexRadch]

I read bad instruction on StackExchange how to sync my fork with parent repository. So I created sync commits in my fork. Now I sync my fork by the right way. To remove strange commits, I removed my fork and recreated it.