feat(engine): include error message of unhandled BPMN error in log #4748
Conversation
Signed-off-by: Sowmya HL <[email protected]>
|
|
|
Hi @mboskamp , hope you are doing good! |
|
Hi @sowmyahl21, thank you for the reminder. I will take a look this week. |
There was a problem hiding this comment.
Hi @sowmyahl21, thank you again for your contribution!
👍 The solution you implemented looks very good already. I added a few hints for the test you wrote. We can make some adjustments to align it better with our testing best practices.
For this review, I used our emoji codes to signal the level of importance:
- 👍 I like that!
- ❓ I have a question. Can you clarify?
- ❌ This has to change.
- 🔧 I have a suggestion that you can take or ignore.
- 🙃 This is nitpicking and should not block progress.
- 💭 I want to start a discussion. Either by sharing my opinion or asking an open question.
| import org.junit.Test; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
| import static org.junit.Assert.*; |
There was a problem hiding this comment.
❌ Please do not use wildcard imports. See our Java coding style guide for more information.
| runtimeService.startProcessInstanceByKey("testProcess"); | ||
| } catch (ProcessEngineException e) { | ||
| // then | ||
| assertTrue(e.getMessage() |
There was a problem hiding this comment.
❌ Please use AssertJ assertions. See our Testing best practices guide for more information.
| "org.camunda.bpm.engine.bpmn.behavior", Level.INFO); | ||
|
|
||
| @Test | ||
| @Deployment(resources = { "org/camunda/bpm/engine/test/bpmn/behavior/UnhandledBpmnError.bpmn20.xml" }) |
There was a problem hiding this comment.
🔧 Convention is to name the BPMN file after your test class and test method. Naming it BpmnBehaviorLoggerTest.testUnhandledBpmnErrorThrowException.bpmn20.xml would allow you to deploy it by just @Deployment without any more parameters. However, you want to reuse this file for both tests, so I would suggest to rename it to BpmnBehaviorLoggerTest.UnhandledBpmnError.bpmn20.xml. This way, it's clear where this file belongs to.
| "Execution with id 'serviceTask' throws an error event with errorCode 'errorCode' and errorMessage 'ouch!', but no error handler was defined")); | ||
| } | ||
| // cleanup | ||
| processEngineConfiguration.setEnableExceptionsAfterUnhandledBpmnError(false); |
There was a problem hiding this comment.
❌ The cleanup code will not run if the test fails for any reason. I know this pattern exists in other tests but it's generally not recommended. Try using the @Before and @After jUnit annotations to set up and restore process engine configuration.
| public void testUnhandledBpmnErrorLogException() { | ||
| String logMessage = "Execution with id 'serviceTask' throws an error event with errorCode 'errorCode' and errorMessage 'ouch!'"; | ||
| runtimeService.startProcessInstanceByKey("testProcess"); | ||
| assertThat(processEngineLoggingRule.getFilteredLog(logMessage)).hasSize(1); |
There was a problem hiding this comment.
❌ Please add // given, // when, // then comments
❌ As per our Testing best practices, please not only assert the count of the result but also the content.
There was a problem hiding this comment.
Hi @mboskamp , regarding this comment to assert content along with the count - in this case, as we are filtering the log based on the given logMessage which asserts the content already along with the count, do we need to explicitly assert the content? please let me know your thoughts.
There was a problem hiding this comment.
👍 Good point, let's keep it like it is and only assert that the result list has size 1.
|
|
||
| @Test | ||
| @Deployment(resources = { "org/camunda/bpm/engine/test/bpmn/behavior/UnhandledBpmnError.bpmn20.xml" }) | ||
| public void testUnhandledBpmnErrorThrowException() { |
There was a problem hiding this comment.
🔧 As per our Testing best practices, please name the tests starting with should followed by a descriptive name e.g. shouldIncludeBpmnErrorMessageInUnhandledBpmnError. The same applies to the other test case.
Related to git issue : #4667
@mboskamp could you please review this PR. Thanks!