spekt / junit.testlogger

JUnit test logger for vstest platform (<= v3.x)
MIT License
77 stars 15 forks source link

Save the XML without BOM mark #18

Closed AnashOommen closed 4 years ago

AnashOommen commented 4 years ago

Currently, the tool generates the xml as utf-8 encoded with BOM mark. Can we do the following?

  1. Encode without a BOM mark. You can pass a UTFEncoding(false) to the XmlTextWriter constructor to achieve this. https://stackoverflow.com/questions/4942825/xdocument-saving-xml-to-file-without-bom?answertab=votes#tab-top

  2. Provide a flag to control this behaviour if this is a breaking change, so I can run this like:

dotnet test --test-adapter-path:. --logger:"junit;LogFilePath=test-result.xml;SkipUtf8Bom=true" test.csproj

Siphonophora commented 4 years ago

@AnashOommen Thanks for pointing this out. For this issue as well as #16 and #17 could you share what you are using to read the xml output? My testing of this library has focused on making sure GitLab is able to parse the files. Best I can tell, it doesn't actually care about the time or date, which may be why this hasn't been an issue so far. I haven't had a chance to test directly with CircleCI or others.

I think a flag would be better. I would be happy with this:

AnashOommen commented 4 years ago

I’m using this against an internal linux-based CI/CD system that expects JUnit-compatible xml output from the tests. I don’t know more details right now.

I strongly suspect it is validating the generated xml against the xsd I linked in one of the bugs, which is why it is failing in my case whereas passes in GitLab. utf-8 with bom encoding is typically the windows way of encoding files, parsers for timestamp for are more flexible than xsd validation, etc.

If it helps, I can upload an xml on Monday and you could validate it against the xsd I linked.

Siphonophora commented 4 years ago

@AnashOommen That make sense. An example wouldn't hurt if you have one.

codito commented 4 years ago

I'm wondering if we should make simple utf-8 without BOM the default format. Unicode spec doesn't recommend BOM. Is there a scenario which would break if we do this?

This stackoverflow question covers some pros/cons of utf-8 with/without BOM: https://stackoverflow.com/a/2223926

Siphonophora commented 4 years ago

For gitlab at least, standard UTF8 isn't a problem. It does seem likely its a safe standard option, I was just being conservative about changing the output.

That said, there was an issue in one of the other loggers requesting a changelog. Seems worth creating one just in case if we change it.

codito commented 4 years ago

+1 to Changelog. Let's provide BOM as an option if anyone would like the old behavior. Like previous releases, we can do a myget release and wait for a few days to see if anything breaks prior to nuget release.

Siphonophora commented 4 years ago

@AnashOommen We decided to switch to plain UTF-8 as the default.

The new pre-release package includes a resolution for this issue, along with #16, #17. If you have time to take a look and provide feedback, that would be great.

AnashOommen commented 4 years ago

Sure, I'll try sometime this week.

On Sat, Feb 15, 2020, 4:32 PM Michael J Conrad notifications@github.com wrote:

@AnashOommen https://github.com/AnashOommen We decided to switch to plain UTF-8 as the default.

The new pre-release package https://www.myget.org/feed/spekt/package/nuget/JunitXml.TestLogger/2.1.29 includes a resolution for this issue, along with #16 https://github.com/spekt/junit.testlogger/issues/16, #17 https://github.com/spekt/junit.testlogger/issues/17. If you have time to take a look and provide feedback, that would be great.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/spekt/junit.testlogger/issues/18?email_source=notifications&email_token=ABCOJCB2HF466OLQ5PEIGB3RDBNQXA5CNFSM4KK77RRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEL3XO5Q#issuecomment-586643318, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABCOJCGFF2VXTS6QMKZALZDRDBNQXANCNFSM4KK77RRA .

Siphonophora commented 4 years ago

@AnashOommen did you ever get a chance to try this? Thanks

Siphonophora commented 4 years ago

Resolved: https://github.com/spekt/junit.testlogger/releases/tag/v2.1.32