Using ‘\n’ instead of Environment.NewLine caused an issue, when all strings except the last one contain ‘\r’ extra character, which leads to property tests failure. I’ve opened a PR, where Rows method adjusted to use system dependent new line characters.
@spirit11 Can you explain a bit more about the issue? I’m interested in it in case it also applies to other tracks.
Unfortunately we cannot just replace the expected values as they’d cause a lot of existing solutions to become invalid.
Okay, I can come up with a test implementation that will leave current solutions valid and more tolerant to the new ones. Does it make sense?
@tasx sure. When you make a solution for this task in a valid way, using Environment.NewLine to concatenate lines or StringBuilder.AppendLine method, line endings have “\r\n” values on a Windows machine. And you have something like that:
“”"
.A.\r\n
B.B\r\n
.A.\r\n
“”"
Or:
“”"
.A.\r\n
B.B\r\n
.A.
“”"
As I see it, Windows workers are used to validate this task on Exercism. So, when tests call an original Rows splitting function with ‘\n’ as a separator it returns following lines for given samples:
[
“.A.\r”,
“B.B\r”,
“.A.\r”,
“”
]
Or:
[
“.A.\r”,
“B.B\r”,
“.A.”
]
I can assume that the first output is considered incorrect, because of an extra line ending at the end, and it can be trimmed or not even added for the last line. But the second one seems to be valid, but won’t be accepted, because the last line isn’t equal to the first due to the extra\r sign.
Even more astonishing is that the behavior is different on Linux and Windows machines, and the output of tests run on Exercism is not very detailed. This could make users stuck on this task. I suggest make tests more tolerant to an input.
I hope, I’ve explained it well. Feel free to ask any questions.
1 Like
What exactly would you suggest we do?
Rows method could be adjusted in a following way:
private static string[] Rows(string x) =>
x.Split('\n').Select(line => line.TrimEnd('\r')).ToArray();
This will leave tests backward compatible and prevent false negatives in case of using another line ending.
That sounds good? Care for a PR?
Sure, recreated this one: Diamond tests adjustment by spirit11 · Pull Request #2345 · exercism/csharp · GitHub
1 Like