Add more String.Split tests

[justinvp]

Round out String.Split tests in anticipation of #1513.

It's not exhaustive. There are some redundant [InlineData(...)] cases that could probably be removed, but it doesn't really hurt to keep them (these tests are fast) as they do still verify the expected result for those counts.

/cc @ellismg

[justinvp]

Hmm, the build seems to have failed for an unrelated reason...

java.io.IOException: remote file operation failed: d:\j\workspace\dotnet_corefx_windows_release_prtest at hudson.remoting.Channel@4a0926b7:dci-win-fbld-6: java.nio.file.AccessDeniedException: d:\j\workspace\dotnet_corefx_windows_release_prtest\bin\obj\Windows_NT.AnyCPU.Release\Microsoft.CSharp

[hpshelton]

Should you also include value.Split(',', options) for completeness?

[justinvp]

You mean, also add the following overload?

public string[] Split(char separator, int count);

This would match the existing char[] overload, but there isn't an existing similar overload that just takes a string[] and a count, hence why it was left off for the new overloads.

[hpshelton]

Sorry, I edited my above comment for correctness.

You have 6 new overloads in the TemporaryStringSplitExtensions but only 5 assertions in a couple of test cases. I think you're missing a couple of assertions since the number of assertions would be a multiple of 6 per test. In most cases in looks like the tests are missing assertions on one of Split(separator) or Split(separator, options) or both.

[justinvp]

This test is verifying that an invalid count throws. The value.Split(',', options) overload doesn't take a count parameter.

[hpshelton]

I love the InlineData usage for these; nice job. I'd add a couple more asserts, but looks good to me. 👍

[weshaggard]

@ellismg FYI our internal test system doesn't support Theory so we have to figure out what to do about these.

[justinvp]

Bummer! I should be able to move all these inline. Let me know.

[ellismg]

Can you keep it as is? I would prefer that we try to write idiomatic tests and have folks like me figure out how to shoehorn them into our internal systems. If it ends up being a pain, I can change this later.

[weshaggard]

Yes this is my attempt to get @ellismg to fix our internal system as opposed to stopping folks from using it.

[weshaggard]

@justinvp looks like you added a good number of tests here, which is great but we want to keep our InnerLoop testing to run in less than 5 seconds. How long does it take to run all the tests in this test project? If it takes longer than that we should consider marking the longer running ones as OuterLoop.

[justinvp]

@weshaggard I haven't timed these on CoreCLR, but on desktop they don't take very long.

[weshaggard]

@justinvp great that should be fine from a timing prospective. Thanks for the data.

[stephentoub]

LGTM

[ellismg]

LGTM as well, thanks for improving coverage here.

[tmat]

So how do I debug these Theory things? Let's say that one of the cases fails and I want to step thru the case?

[justinvp]

@tmat:

So how do I debug these Theory things? Let's say that one of the cases fails and I want to step thru the case?

Set a (conditional) breakpoint in the [Theory] method.

[tmat]

I see, so I need to rewrite the content of the InlineData as a condition of the BP. Slightly annoying if it needs to be done multiple times. I fail to see the benefit of this attribute over just a sequence of calls to a test helper method:

TestSplit(",", ',', 0, StringSplitOptions.None, new string[0]);
TestSplit(",", ',', 1, StringSplitOptions.None, new[] { "," });
...

[stephentoub]

The primary benefit of [Theory] from my perspective is that each invocation is treated as its own test, and the xunit tracing of failed tests will include all of the parameter/inline data information for the case that failed. This leads to much better debugging ability when the debugger isn't attached during test execution (in particular when source/line information isn't available).

[tmat]

@stephentoub Under what circumstances do you not have line information?

[tmat]

@stephentoub Once your test suite grows a lot, you make a change that affects a lot of tests (likely not this particular case) and need to update the baseline it is actually better to have a single test failure reported in the output than a lot of failed test cases.

[stephentoub]

Under what circumstances do you not have line information?

On Linux/OSX, at least that's the state of things today. But even if it is available, I find it quite helpful to see in the traced failure information exactly what combination of inputs caused things to fail, rather than having to manually track back through each line in the stack trace to see what the various inputs.

With regards to setting breakpoints for debugging, it's also trivial to temporarily comment out all but the [InlineData] elements one cares about, if setting conditional breakpoints is too cumbersome.

need to update the baseline it actually better to have a single test failure

That assumes your baseline is being tracked separately from the source. If you do something like [ActiveIssue] as in corefx, it's still only applied in one place.

[tmat]

I see. Fair points. I guess we have never needed this since we are able to run tests from VS by right-clicking on them.

"That assumes your baseline is being tracked separately from the source." That's not what I assume. Your baseline is in arguments of the helper:

TestSplit(",", ',', 0, StringSplitOptions.None, new string[0]);
TestSplit(",", ',', 1, StringSplitOptions.None, new[] { "," });

What I meant is that if you have 10 test failures and each has 20 cases, you get 10 errors in output not 200. You'll also get the arguments to the assert equals, so you actually know what values were involved.

[stephentoub]

You'll also get the arguments to the assert equals, so you actually know what failed.

Sure, but often such an assert is deep in the weeds and by itself isn't enough to tell you what the inputs to the higher level test were. You also don't get such help with other asserts like Assert.True.

you get 10 errors in output not 200.

Yes, but there's a flip side to that. Many times in the past I've been bitten thinking I only have one failure left, then I fix it, and once I do more then show up that were hiding behind the previous failure; they weren't getting executed because an assert from a previous test case was firing and preventing their execution.

There are certainly pros and cons to the approach, and I don't think it's a one-size-fits-all, but in a case like this I think it makes a lot of sense.