victorolinasc / junit-formatter

A JUnit XML report exporter for Elixir's ExUnit
https://hexdocs.pm/junit_formatter/
Apache License 2.0
110 stars 37 forks source link

Cannot set absolute path #15

Closed dantswain closed 7 years ago

dantswain commented 8 years ago

The default location for output for me is _build/test/lib/<app> relative to the project root (where I run mix test from) - I'm not sure that's intended?

Regardless, I wanted to change it to an absolute path so that I could configure Jenkins without having to know the build path. I tried:

config :junit_formatter,
  print_report_file: true,
  report_file: Path.expand("../test-output.xml", __DIR__)

But this causes the output to go to _build/test/lib/<app>//<intended path> - i.e., the path is just appended. From the source, append is exactly what's happening https://github.com/victorolinasc/junit-formatter/blob/master/lib/formatter.ex#L138

victorolinasc commented 8 years ago

Sorry for the delay @dantswain!

This is actually intended. The path of the reports are usually inside the build path because they should be cleaned with the build files. If we allow to generate it to any path this would make the chore of cleaning them more difficult. Probably you would end up with stale reports and that is not good.

In Jenkins, the build path normally is relative to the $WORKSPACE variable (if your project is at the root of your repo than your all good). Does that work for you?

dantswain commented 8 years ago

I understand what you're saying, and I guess I just disagree. It's confusing to have a report_file option that doesn't actually set the path of the report file :/

I'm able to get it to work in Jenkins by using ./_build/test_lib/<myapp>/test-output.xml when copying out the artifact, but that's sort of suboptimal IMHO because it means (conceptually) Jenkins has to know about the structure of the build, and it's a little different for each project. I discovered this because I was trying to set up a handful of Elixir projects in Jenkins and kept accidentally copying and pasting the test result artifact line and forgetting to change the location of the xml file :( Or, to reword that in a way that makes it less about my mistakes - it doesn't really promote code reuse in the CI environment ;)

Personally, I would rather have control over the output location and be responsible for my own cleanup. The default could still be inside the _build directory and there's nothing to stop people from keeping the file there if easy cleanup is a high priority. I haven't tried it yet, but is there anything stopping me from using a relative path that puts the file outside of _build? I.e., it would still be possible for me to change the output location, so why make it unduly difficult?

If you want to stand by the design you have, I'll understand and you can close this issue. Just my two cents. (Though at least maybe document the way that report_file works, since it's not intuitive? I could help with a PR... )

victorolinasc commented 8 years ago

I agree with you about documentation. The option was meant initially to customize the name of the file and not the path of it.

I am not against letting people customize the path. Yours is a valid case and perhaps that can be set in a different variable to allow for both and not having to deal with logic like "if the name starts iwth a slash or a schema like C: it is a path otherwise it is just the file name".

Some other valid points might be umbrella apps. Just want to stress the idea before developing it. Your case uses umbrella apps? I'd have to dig a bit further at what happens when mix test is run from the root of an umbrella project. Not sure config would be overwritten.

I will leave this open and look into implementing it. Will probably go the different variable way though.

erez-rabih commented 7 years ago

Just wanted to remark that I also need to output the test result to a different location which is not relative to the build path. I'd love to see this option added.

cs-victor-nascimento commented 7 years ago

This is a small change. I will see to it this weekend.

cs-victor-nascimento commented 7 years ago

This has been release as of 1.2.0 :)

Thanks once again for the feedback.

erez-rabih commented 7 years ago

@cs-victor-nascimento thanks for the fast update