Skip to content
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

Add XML prolog to junit output #72

Merged
merged 6 commits into from
Jan 24, 2019
Merged

Conversation

lalugeo
Copy link
Contributor

@lalugeo lalugeo commented Jan 10, 2019

Issue:

Many XML parsers require the XML Prolog at the beginning of the xml file to start parsing.

Change proposed

Add an optional feature to control whether the generated XML should contain a XML prolog or not.This is controlled with the newly created flag called includeXmlProlog as the package.json key or JEST_JUNIT_INCLUDE_XML_PROLOG as the environment variable.
Include the declaration of the XML prolog persistently in the output always

Before

<testsuites name="jest tests">
  <testsuite name="addition" tests="1" errors="0" failures="0" skipped="0" timestamp="2017-07-13T09:42:28" time="0.161">
    <testcase classname="ADDITION POSITIVE NUMBERS" name="addition positive numbers should add up" time="0.004">
    </testcase>
  </testsuite>
</testsuites>

After

<?xml version="1.0" encoding="UTF-8"?>
<testsuites name="jest tests">
  <testsuite name="addition" tests="1" errors="0" failures="0" skipped="0" timestamp="2017-07-13T09:42:28" time="0.161">
    <testcase classname="ADDITION POSITIVE NUMBERS" name="addition positive numbers should add up" time="0.004">
    </testcase>
  </testsuite>
</testsuites>

* new string constant and options added for prolog
* README updated with new options
@palmerj3
Copy link
Collaborator

Hey @lalugeo thanks for submitting this!

Can you just briefly describe what this PR achieves? Are you having an issue with the generated XML?

@lalugeo
Copy link
Contributor Author

lalugeo commented Jan 10, 2019

Hey @lalugeo thanks for submitting this!

Can you just briefly describe what this PR achieves? Are you having an issue with the generated XML?

Thank you for the quick response. But sorry, i was just editing the PR to include my comments 😊

@palmerj3
Copy link
Collaborator

@lalugeo ok that seems reasonable. Given this do you see any risk in this simply being hardcoded and not a CLI option?

So we would always have a prolog.

@lalugeo
Copy link
Contributor Author

lalugeo commented Jan 10, 2019

The risk might be to any text based or regex based parsing being done by the consumers of this package on the output xml file. In such scenarios, this PR would break their implementations. Hence I added a flag that has default value of false, causing the prolog to be not added to the output. So if any consumer of this package, needs the prolog, they have to specifically turn on this flag.

But of course, to keep it simple, we could always harcode it without any of the options, which would cause the prolog to be returned always.

@palmerj3
Copy link
Collaborator

I'm not too worried about people who parse these with regex when we generate valid xml.

If you wouldn't mind, I think I would rather see this prolog hard coded so we reduce the configuration options.

@lalugeo
Copy link
Contributor Author

lalugeo commented Jan 10, 2019

Sure, i have made those changes.

@palmerj3
Copy link
Collaborator

@lalugeo thank you!

I'm sorry I don't mean to nitpick. We're very close to just merging this.

I feel it would be a lot cleaner if we used the xml lib to add this prolog instead of string concat.

See "Declaration example" here https://github.com/dylang/node-xml

Mind making that change?

@lalugeo
Copy link
Contributor Author

lalugeo commented Jan 11, 2019

Yes, you are right, this is a way more cleaner option. Thanks for pointing me out in that direction.

Also, do you think the xml sample outputs in the README should be updated with the new prolog lines too?

@lalugeo
Copy link
Contributor Author

lalugeo commented Jan 15, 2019

@palmerj3 , any updates on the merge?

@palmerj3
Copy link
Collaborator

@lalugeo hey I'm sorry I've been quite busy this week. I will get to this tomorrow.

@palmerj3 palmerj3 merged commit 860f7ef into jest-community:master Jan 24, 2019
@palmerj3
Copy link
Collaborator

@lalugeo this has been merged and I published jest-junit 6.1.0 which includes this. Please let me know if there are any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants