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

Specs: JUnit formatter #2455

Merged
merged 3 commits into from
May 31, 2016
Merged

Conversation

juanedi
Copy link
Contributor

@juanedi juanedi commented Apr 14, 2016

This PR adds the option to enable JUnit xml output to a file while running specs. This allows more detailed reporting of tests and API access to results in CircleCI. See:

https://circleci.com/blog/announcing-detailed-test-failure-reporting/
https://circleci.com/docs/test-metadata/

Usage:

# circle.yml
test:
  override:
    - mkdir "$CIRCLE_TEST_REPORTS/crystal"
    - crystal spec --junit_output "$CIRCLE_TEST_REPORTS/crystal/junit.xml"

This will displayed as:
screen shot 2016-04-14 at 5 19 28 pm

Also, this could be useful to integrate with other CI services in the future, since this format seems to be a de facto standard for reporting test results.

@luislavena
Copy link
Contributor

Could this be done as --formatter junit instead of a custom parameter? I think we will see more requests for other formatters that will add too many configuration flags.

As for the XML escape/substitution, perhaps the HTML ones can be reused instead?

https://github.com/crystal-lang/crystal/blob/master/src/html.cr

@juanedi
Copy link
Contributor Author

juanedi commented Apr 14, 2016

--formatter junit looks great. One thing though: this formatter takes an output_directory as paremeter.
We could add a second --output/-o option, but (for now) it would only make sense when this formatter is used.

Future formatters might also take custom parameters too. I'm not sure what is the most extensible and consistent way of handling this.

@juanedi
Copy link
Contributor Author

juanedi commented Apr 14, 2016

Regarding XML escaping: I'm not an expert on this but most of the substitutions used for HTML.escape don't seem to be necessary when escaping XML.

I wouldn't do it just for code reuse... it's just a gsub. More important: I don't think it's harmless to perform those extra substitutions... how would the program/person reading the escaped text know we made those transformations?

Am I missing something?

@luislavena
Copy link
Contributor

More important: I don't think it's harmless to perform those extra substitutions

You're correct, for a moment I thought that the substitution list looked lot like HTML one, but seems is very specific to HTML than the XML counterpart. My apologies.

As for the formatter, perhaps an standard can be defined? say the formatter requires a parameter/directory, so:

$ crystal spec --formatter junit=path/ --formatter other

And path/ is extracted and used when initializing the formatter. But of course that depends on how many formatters with these options will be required (YAGNI).

Cheers.

@jhass
Copy link
Member

jhass commented May 29, 2016

I think the current option is fine, especially since it's something that should happen additionally, not replacing the current formatter. We can look into cleaning up the options once we do have some formatters and a better overall picture on their needs.

Could you please rebase?

@asterite
Copy link
Member

@jhass I agree. I wanted to think of a better way to integrate this, like using --formatter, but we can merge this now and later improve it.

@juanedi juanedi force-pushed the spec-junit-formatter branch from a14eb02 to c2d903e Compare May 31, 2016 14:23
@juanedi
Copy link
Contributor Author

juanedi commented May 31, 2016

Rebased :)

@jhass
Copy link
Member

jhass commented May 31, 2016

Cool, could you squash (git rebase -i master) the last two commits into 6cdf655?

@juanedi juanedi force-pushed the spec-junit-formatter branch from 10107f9 to b9bca94 Compare May 31, 2016 19:30
@juanedi
Copy link
Contributor Author

juanedi commented May 31, 2016

Done!

@jhass
Copy link
Member

jhass commented May 31, 2016

Unfortunately not, make sure to pass -i to git rebase, then move the two lines for the last two commits below the one for (now) 2d5c10e, then change for the two ones pick to fix or squash.

@juanedi juanedi force-pushed the spec-junit-formatter branch from b9bca94 to fe1ffda Compare May 31, 2016 19:59
@juanedi
Copy link
Contributor Author

juanedi commented May 31, 2016

Sorry, I thought you just wanted me to update instead of cleaning up the history :)
It should be done now.

@jhass
Copy link
Member

jhass commented May 31, 2016

Uh, I liked the separate commits for the separate stuff, I just wanted the last two go into the one introducing what they're fixing :( One more try? git reset --hard b9bca941dff756833cd582a6adbacdafaed906d6 should get you back the previous history.

juanedi added 2 commits May 31, 2016 17:13
Reporting in this format allows interoperability with CI tools such as Circle
This caused an error in a JUnitFormatter, which was assuming the spec name was a valid string.
@juanedi juanedi force-pushed the spec-junit-formatter branch from fe1ffda to 6fd1d60 Compare May 31, 2016 20:13
@jhass
Copy link
Member

jhass commented May 31, 2016

Perfect, thank you!

@juanedi
Copy link
Contributor Author

juanedi commented May 31, 2016

I almost sure I got it right this time :-P

@jhass jhass merged commit 8670c45 into crystal-lang:master May 31, 2016
@keplersj keplersj mentioned this pull request Jun 1, 2016
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.

4 participants