Try to differentiate between null and empty strings in test theories

[Numpsy]

refs #493

Some testing to pin down where the issue is - not sure if this is the best apporach to fixing it, or if there are other types than strings that might have similar issues.

[farlee2121]

Overall, looks good. Gives us a place to make changes if we find any more special values that need differentiated.

I made one change: I grouped the new tests in an explanatory test list.

Checking off a few other potential special values

• white space should produce different values
• numeric types... I can't think of any issues
• weird chars... zero-width characters?
• reference types... this could be a problem. Classes don't have good default string representations

Example of breaking reference type

type ClassA(i: int) = 
  let i = i

testTheory "I am theory" [ClassA(5); ClassA(4)] (fun (refType: ClassA) -> true)

I'm not sure how common this reference type scenario is. The empty vs null one seems quite common though, and it's worth getting a fix in for that at least.

[farlee2121]

Also, thanks for the PR!

[Numpsy]

• white space should produce different values

With the current change, it works to run the tests when the value is just whitespace, but the Visual Studio test explorer looks a bit 'unfriendly' -

If I borrow the quoting approach from NUnit

if value = Unchecked.defaultof<'T> then "null" else $"\"{value}\""

then it looks like this, which seems nicer:

[Numpsy]

weird chars... zero-width characters?

I don't know if there are any potential locale specific issues that might occur?

reference types... this could be a problem. Classes don't have good default string representations

I can't recall offhand what NUnit/XUnit do for that, will have to test.

[farlee2121]

Using NUnit and XUnit theories as a reference is a great idea

[Numpsy]

Ok, I don't think you can specify class instances in the NUnit TestCase attribute, and if I use TestCaseSource then I get this, where it only lists one test but then has two results, and the display name in the test explorer is just the class type -

Visual Studio:

Ionide:

[farlee2121]

C# attributes can only accept constant parameters. Makes sense that class instances are out.

That's interesting that TestCaseSource behaves differently than the attributes. That still leaves us with an open question on how to handle classes.

Did you check out xUnit too?

[Numpsy]

just tried adding a 'TheoryData' based test to an Xunit based test project and got

So it's still showing one test with several results, but in that case it's managing to show a string representation of the class instance instead of just showing the type name

[farlee2121]

Thanks for the research!

Maybe we could mimic the xUnit approach to class rendering. That could open new issues with special characters in test names though... We could potentially detect duplicate test names and merge them into a single test with the cases in the description, but that feels like an unintuitive user experience.

I think it's worth merging the string/null fix by itself, since that's a fairly common issue. It's less clear how common the class issue would be. I also doubled checked that records and unions have sensible default string representations and wouldn't suffer from the same issue as classes.

[Numpsy]

It's less clear how common the class issue would be.

I don't recall having used classes with TestCase myself (it's usually base types like strings, numbers, enums). so making the string case work fixes my problem.

That could open new issues with special characters in test names though We could potentially detect duplicate test names and merge them into a single test with the cases in the description

I'm not sure if putting that data in the test names would have issues if the class instances had dynamic fields in them? (e.g. say you had a test class instance with a time stamp or guid or some such property in it, trying to render that as part of the test name rather than just in the results might be problematic?)

[farlee2121]

The characters that cause issues (that I'm aware of) are + and ., which are used as separators for test segments. I don't think guids or dates would typically contain those.

I've also seen some filter issues with characters like ()&|, but it's on the test explorer to escape those kinds of characters.

[farlee2121]

Oof. Glad I thought to test before pushing the packages. It looks like quotes around strings causes issues with the Ionide test explorer. I'll see if it can be fixed

[farlee2121]

A fix is PRed https://github.com/ionide/ionide-vscode-fsharp/pull/1999

[Numpsy]

Oof. Glad I thought to test before pushing the packages. It looks like quotes around strings causes issues with the Ionide test explorer. I'll see if it can be fixed

Sorry, thought I'd tested that when I made the screen shots, must not have though :-(

My comment about dates etc was more about 'dynamic' data, as in what will the test explorer do if the test name were made of data that might be different between runs (maybe not common, but could happen with class types?) - although maybe you could have a + in the string for a time value as an offset from UTC.

Your comment about . made me think about floating point numbers as well though - which currently look like this in Ionide:

[Numpsy]

• reference types... this could be a problem. Classes don't have good default string representations

Just accidentally fell over a related situation here - If I try to give a testTheory arrays of strings as inputs, then each array has the same string representation so you get a Found duplicated test names error again

[farlee2121]

While not very elegant, a workaround could be to give each case a data point to differentiate it. I.e. [("case1", [array here]); ("case2", [other array]).

Actually, we could potentially generalize such an approach. Automatically add a case identifier to make sure each case is unique even if the arguments don't have distinct string representations.