-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Unicode string test failure output #47
Conversation
…copy of string if it has unicode characters in it.
We seem to have multiple copies of |
How does it look if the strings are long or contain new lines? |
Added a newline test (\n vs \r\n), those are typed out as their backslash escapes (this is how elm types them out in toString). Also added a very long line, hard to read test output in my terminal because of word wrap, but very easy without word wrap. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small nits, otherwise looks good.
tests/src/Runner/String/Format.elm
Outdated
"f" | ||
|
||
_ -> | ||
String.fromInt (i |> remainderBy 16) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to call remainderBy
again. decimalDigit -> String.fromInt decimalDigit
tests/src/Runner/String/Format.elm
Outdated
|> List.map Char.toCode | ||
|> List.map | ||
(\c -> | ||
if isAsciiChar c == False then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use if not (isAsciiChar c) then
or flip the conditional.
Better late than never, I guess. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
-- \_ -> "first\u{000a}second" |> Expect.equal "first\r\nsecond" | ||
-- , test "long lines" <| | ||
-- \_ -> "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Quisque in scelerisque arcu. Curabitur cursus efficitur turpis sed porttitor. Class aptent taciti sociosqu ad litora torquent per conubia nostra, per inceptos himenaeos. Nunc eu cursus ex. Proin accumsan quam quis dui semper finibus. Nunc vel nibh at tellus finibus rhoncus eu eget dolor. Sed eget neque ut lorem imperdiet fermentum. 😻 Morbi iaculis ante euismod, vulputate velit ut, varius velit. Nulla tempus dapibus mattis. In tempus, nisi a porta lobortis, nulla lacus iaculis quam, vel euismod magna risus at tortor. Integer porta urna odio. Nulla pellentesque dictum maximus. Donec auctor urna nec tortor imperdiet varius." |> Expect.equal "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Quisque in scelerisque arcu. Curabitur cursus efficitur turpis sed porttitor. Class aptent taciti sociosqu ad litora torquent per conubia nostra, per inceptos himenaeos. Nunc eu cursus ex. Proin accumsan quam quis dui semper finibus. Nunc vel nibh at tellus finibus rhoncus eu eget dolor. Sed eget neque ut lorem imperdiet fermentum. Morbi iaculis ante euismod, vulputate velit ut, varius velit. Nulla tempus dapibus mattis. In tempus, nisi a porta lobortis, nulla lacus iaculis quam, vel euismod magna risus at tortor. Integer porta urna odio. Nulla pellentesque dictum maximus. Donec auctor urna nec tortor imperdiet varius." | ||
-- ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these tests commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're the tests used to show the output of failed string equality expectations. I don't know how to write meaningful automatic tests that check that the output remains the same.
@@ -135,29 +203,64 @@ equalityToString : { operation : String, expected : String, actual : String } -> | |||
equalityToString { operation, expected, actual } = | |||
-- TODO make sure this looks reasonable for multiline strings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this pull request solves this TODO. 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately not :/ Long strings with automatic line wrapping still look weird.
This adds a unicode-escaped copy of any failing string that contains non-standard ascii characters.
I'm not completely happy with the formatting, so if you have suggestions, please do tell. Otherwise, I think it's still much better than what we currently have. I'm comparing the escaped strings, not character by character, which is why we're getting diffs that cover part of an escape sequence.
Solves elm-community/elm-test#225 and #38.