search

victorolinasc / junit-formatter

A JUnit XML report exporter for Elixir's ExUnit
https://hexdocs.pm/junit_formatter/
Apache License 2.0
111 stars 37 forks source link

Handle unicode in test names #32

Closed sparta-developers closed 6 years ago

sparta-developers commented 6 years ago

Atom.to_charlist does not handle unicode well, but Atom.to_string seems to.

Atoms can include unicode as of a recent version of Elixir. We had added a ≤ in the name of one of our tests recently, which caused the xml file output by junit-formatter to be empty.

cs-victor-nascimento commented 6 years ago

Awesome catch :)

Let's wait for CI and I will merge it. THanks a lot.

sparta-developers commented 6 years ago

Hmm... it looks like there may be another issue. The XML is generated with the ISO-8859 character encoding, instead of UTF-8. We're looking into it now, to see if we can output the XML file with a different character encoding.

sparta-developers commented 6 years ago

CI is almost certainly failing because UTF-8 support for atoms was added in Erlang 20.

You may not want to merge this, depending on what versions of Erlang you want to support. The documentation for Elixir 1.6 implies that the project does want to support Erlang 19, even though features like unicode in atom will raise errors.

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 94.545% when pulling 66fec3c51a7639b278d8b83bc4b70c67c39293b9 on sparta-science:master into 46dce5154896d1801a10e418f9c6281402ad2c18 on victorolinasc:master.

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 94.545% when pulling 8056cfd2c2a6ba38e781148e1f4904b831ba5ab2 on sparta-science:master into 46dce5154896d1801a10e418f9c6281402ad2c18 on victorolinasc:master.

victorolinasc commented 6 years ago

I'm setting up different erlang versions with asdf to make some more tests. On OTP 20 everything looks good but if it breaks OTP 19 then we can't merge this unfortunately.

I will see if I can workaround this.

Thanks for bringing this up though!

sparta-developers commented 6 years ago

Hmmm... so I think that the code changes are fine in both Erlang 19 and 20, it's the test that we added that is problematic. UTF-8 characters in test names will break in Erlang 19 regardless of how the formatter handles them.

We could try to make the test run conditionally based on Erlang version (if that's even possible), or we could remove 3 ≤ 4 test and trust the rest of the test suite to show that these changes don't break the formatter in general.

Do you have any preferences?

victorolinasc commented 6 years ago

I think this could be an issue in ExUnit itself. If a string is not valid for a name on conversion to atom then it should give a better error than simply argument error or handle it there (and defenitely not here o junit-formatter).

I think I will open an issue there and perhaps open a PR there to handle this on ExUnit itself. What is your opinion?

If this is ok, then I'm fine with removing the test and just ensuring this does not break current implementation.

victorolinasc commented 6 years ago

I've opened the issue here.

You can remove the test for now.

Thanks!

victorolinasc commented 6 years ago

I will merge this and resolve locally.