Spec::JUnitFormatter enhancements

[Sija]

• Outputs elapsed time for testsuite and single testcases
• Outputs timestamp for testsuite
• Outputs hostname for testsuite
• Outputs exception class name as an <error type="..."> attribute
• Escapes control characters within <testcase name="..."> attribute
• Marks pending testcases as skipped
• Returns count for skipped testcases (from pending)
• <testcase classname="..."> is now built from relative path (to Dir.current)
• Changes —junit_output flag to take full path to the junit xml file, instead of just a directory

[Sija]

Sorry for the ongoing changes, fwiw this is ready for review.

[RX14]

Thank you!

[Sija]

Last commit should fix #8617 (comment) - and it does :)

[Sija]

Additional commit is to help fixing #8617 (comment) - without it, we'd need to aggregate xml files from all of the runs into a single directory, whereas with --junit_output_path it's possible to do the same but directly. For BC reasons I've opted for adding another flag instead of changing existing --junit_output.

[RX14]

As far as I can tell, --junit_output_path tries to open the directory as a file...

[Sija]

@RX14 no, why?

crystal/src/spec.cr

Lines 132 to 139 in 97cb7b4

	opts.on("--junit_output OUTPUT_DIR", "generate JUnit XML output within the given OUTPUT_DIR") do |output_dir|
	junit_formatter = Spec::JUnitFormatter.file(Path[output_dir, "output.xml"])
	Spec.add_formatter(junit_formatter)
	end
	opts.on("--junit_output_path OUTPUT_PATH", "generate JUnit XML output under the given OUTPUT_PATH") do |output_path|
	junit_formatter = Spec::JUnitFormatter.file(Path[output_path])
	Spec.add_formatter(junit_formatter)
	end

crystal/src/spec/junit_formatter.cr

Lines 37 to 41 in 97cb7b4

	def self.file(output_path : Path)
	Dir.mkdir_p(output_path.dirname)
	file = File.new(output_path, "w")
	JUnitFormatter.new(file)
	end

just to confirm that passing directory will blow things up as it should:

File.new(Path["."], "w") # => raises Errno (Error opening file '.' with mode 'w': Is a directory)

[RX14]

oh, right, it's the old code which hardcoded output.xml, my bad

I'd rather change the semantics of the existing option. It's stupid as it is.

[Sija]

I'd rather change the semantics of the existing option. It's stupid as it is.

I was thinking the same, addressed in the last commit.

[bcardiff]

I'm having second thoughts on the following change

• Changes —junit_output flag to take full path to the junit xml file, instead of just a directory

I checked other tools at the usual configuration is specifying a directory. As it was before.
Using a directory will allow future splitting in different files.

It seems that this change was motivated to grab the result of the splitted output of the std_specs, if so I would prefer to revert this part of the change and use different directories for the parts.

Am I missing something @Sija ?

[Sija]

I checked other tools at the usual configuration is specifying a directory. As it was before.

What other tools you have in mind?
https://github.com/sj26/rspec_junit_formatter as an example takes a full path, not a directory.

Also, it as @RX14 already mentioned, it doesn't make much sense to take only directory as an argument. Why won't users be allowed to name they files as they wish?

[bcardiff]

• gradle uses a main configuration testReportDir
• junit has junit.outdir

I would prefer to have another cli option to indicate file (or file pattern) to be used within the output directory. The file pattern could be used to split results based , but scope creep.

This idea does not necesesarily requires a new option, but then at least what I want is that if the path option does not end up with .xml keep the old behavior and use it as a folder. with a fixed output.xml or results.xml.

[RX14]

If we're going to revert behaviur i'd at least want an explanation for why these other tools do this.

Are we ever going to emit multiple files? What's the benefit.

[bcardiff]

Splitting files is useful for reporting purposes. If the spec is used for acceptance test and they are grouped doing different files per group my help. Or it can be splitted by test result status directly. Or by namespace.

I can't say why other tools work like that. But I found it odd the change. And i discover becure the CircleCI test result integration broke on nightly on some projects.

If the path does not ends with .xml it should behave as before. The previous behaviour wasn't broken.