-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Spec::JUnitFormatter: Allow output directory as --junit_output #8692
Conversation
Why not just append Until there's actually any way, or need, to generate multiple files, I'd prefer it stay as it is. |
Because it keeps compatible with the previous behaviour without ambiguity. |
Nobody has explained why splitting the results would be an improvement, and we've already made the breaking change in a release. I see no benefit to previous behaviour at this point. |
This is preventing the breaking change on master. There was no tagged release with that behavior. I mentioned here some scenarios for splitting #8599 (comment) . |
Right, sorry, I thought that'd made it into a release somehow. |
if output_path.extension != ".xml" | ||
output_path = output_path.join("output.xml") | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That belongs to src/spec.cr
, not here.
Lines 132 to 135 in 42a6490
opts.on("--junit_output OUTPUT_PATH", "generate JUnit XML output within the given OUTPUT_PATH") do |output_path| | |
junit_formatter = Spec::JUnitFormatter.file(Path.new(output_path)) | |
Spec.add_formatter(junit_formatter) | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree. It depends on the semantics of the argument.
One could say that the creation of the parent directory also belongs to the CLI.
The .file
methods acts as a helper, the real deal is the initialize(IO)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO .file
is expecting a path to... the file - not some arbitrary path to mutate afterwards - the whole logic of "tweaking" the path should be done outside - in this case by spec itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me is a path configuration of the output formatter. If there is splitting how to split and which file to create would belong in the formatter and not spec itself since it is not something shared with other formatters like DotFormatter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[...] since it is not something shared with other formatters like DotFormatter.
That's exactly my point, formatter itself shouldn't be responsible for doing any magic with the given path. If I'm calling Spec::JUnitFormatter.file(path)
I'd expect to pass a file path to it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like it, but it's restoring previous behaviour so
@RX14 I agree, it's not ideal solution. At some point we need to figure out if writing to different files is actually going to happen and we can reconsider and if necessary break things then. |
I'd prefer to rule out ever doing that sooner rather than later, as a form of YAGNI simplification. Another option can always be added in the (unlikely) case anyone ever fleshes out some rules to generate multiple XML documents. I think most people are just going to have multiple top-level spec suite files if they want to do this. And I don't think they ever will actually want to do this. |
Follow up of #8599 (comment)