azotcsit edited a comment on pull request #171:
URL: https://github.com/apache/ant/pull/171#issuecomment-971481737
Hi @jaikiran, I'm new to this codebase, so I'm sorry if I missed something.
If we talk hypothetically then not only `resultFile` attribute applicable to
formatters, but also `outputDir` and `extension` (not mentioning
`useLegacyReportingName`) because `TestExecutionListener` does not define any
output format and a person may want to write test results to other destination
than a file (e.g. an external database).
I feel the current design mixes up `TestExecutionListener` and
`TestResultFormatter`. On the one hand we want to keep `TestExecutionListener`
clean of anything that limits it, on the other hand we want to be able to pass
some arguments to the formatter.
> a custom implementation of
org.junit.platform.launcher.TestExecutionListener can decide where to write out
the output generated by the listener
I agree that it **can** decide, but according to the current design it is
supposed to use `TestResultFormatter.setDestination(OutputStream os)` and any
other way seems to be a hack of the system rather than an expected solution.
With that being said, I have some questions. The answers would help me to
understand what changes to bring back to _ant_ and what to keep in custom
foratters. Also it will probably prevent me of raising controversial tickets
like this one.
Architectural questions:
1. Have you considered to introduce `LegacyFormatterDefinition`? So whoever
want to stick to old formatters approach (I guess most of people) they can
easily do that. As a result, `ListenerDefinition` is kept clean.
2. I feel there is a need to decouple output part of the listener and that
should solve all the problems. Smth like that:
```
<listener class="xml-writer" writerClass="com.ant.junitlauncher.FileWriter">
<outDir>bla-blah<outDir/>
<extension>extension<extension/>
</listener>
<listener class="brief" writerClass="com.ant.junitlauncher.StdOutWriter" />
```
That should help to keep listeners and `LauncherSupport` clean of any output
logic. Also it would let customize output in any way the implementation
dictates. WDYT?
Other questions:
1. Is it currently possible pass an attribute to a custom formatter (without
it being defined on listener level)? Would smth like system properties work?
2. I understand you consider existing formatters being legacy but I believe
they are what people actively use right now. So it would be nice to provide at
least comparable functionality to what was available for _ant-junit_. I
definitely can customize `TestResultFormatter.setDestination(OutputStream os)`
and that would solve the problem of writing to `stdout`, but I'm just wondering
whether we need to support it for the community use. Maybe we can just put a
"legacy" prefix (`legacyUseFily`) to the new attribute to emphasize it is
something not really right. WDYT?
3. Is there a reason why `TestPlan` field is not a part of
`AbstractJUnitResultFormatter` class? The field is defined in all existing
formatters, I'm just wondering why not to move that because it seems to be
applicable to all possible formatters.
4. Is there a reason why `useLegacyReportingName` is not a part of
`AbstractJUnitResultFormatter`? Since `TestResultFormatter` has
`setUseLegacyReportingName` method it seems to be reasonable to move
`useLegacyReportingName` there as well.
Sorry for bugging you with so many questions :smile:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org
