tpalfy commented on a change in pull request #5093:
URL: https://github.com/apache/nifi/pull/5093#discussion_r639668296
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/processor/SimpleProcessLogger.java
##########
@@ -448,4 +454,20 @@ public void log(LogLevel level, String msg, Object[] os,
Throwable t) {
}
}
+ private String getCauses(final Throwable throwable) {
Review comment:
We could leverage Java for more readability to reverse the order of the
error messages. Something like this:
```java
final LinkedList<String> causes = new LinkedList<>();
for (Throwable t = throwable; t != null; t = t.getCause()) {
causes.push(t.toString());
}
Collections.reverse(causes);
return
causes.stream().collect(Collectors.joining(System.lineSeparator() + CAUSES));
```
Also this implementation adds a new line at the start. Is that intentional?
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-components/src/test/java/org/apache/nifi/processor/TestSimpleProcessLogger.java
##########
@@ -68,7 +68,7 @@ public void before() {
@Test
public void validateDelegateLoggerReceivesThrowableToStringOnError() {
componentLog.error("Hello {}", e);
- verify(logger, times(1)).error(anyString(), eq(task),
eq(e.toString()), eq(e));
+ verify(logger, times(1)).error(anyString(), eq(task), anyString(),
eq(e));
Review comment:
I think these modifications are insufficient to test the added
functionality.
We don't necessarily need to check the full error message in all the tests -
i.e. the `anyString()` may be okay in these ones, but then we should add new
tests.
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
