Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix: Test-Context Error handling is not graceful #1122 #1123

Closed
wants to merge 1 commit into from
Closed

Fix: Test-Context Error handling is not graceful #1122 #1123

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions core/citrus-api/src/main/java/org/citrusframework/context/TestContext.java
View file
Open in desktop
Expand Up @@ -432,7 +432,7 @@ public String resolveDynamicValue(String expression) {
* @param cause
* @return
*/
public CitrusRuntimeException handleError(String testName, String packageName, String message, Exception cause) {
public RuntimeException handleError(String testName, String packageName, String message, Exception cause) {
// Create empty dummy test case for logging purpose
TestCase dummyTest = new EmptyTestCase(testName, packageName);

Expand All @@ -443,7 +443,7 @@ public CitrusRuntimeException handleError(String testName, String packageName, S
testListeners.onTestFailure(dummyTest, exception);
testListeners.onTestFinish(dummyTest);

return exception;
return new RuntimeException(cause);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

welcome, new contributor! before anything: thanks for your help - much appreciated!

however, that is not exactly what I meant within #1122 😉 the signature of the method was just fine. on the other hand, I would expect this CitrusRuntimeException to be returned, no matter what. but here come the problematic lines:

// inform test listeners with failed test
testListeners.onTestStart(dummyTest);
testListeners.onTestFailure(dummyTest, exception);
testListeners.onTestFinish(dummyTest);

if you want to take on that issue, I would suggest starting with the following test in TestContextTest (note that it fails right now):

@Test
public void handleErrorDespiteListenerFailure() {
    var testListeners = new TestListeners();

    var testStartListenerMock = mock(TestListener.class);
    testListeners.addTestListener(testStartListenerMock);

    context.setTestListeners(testListeners);

    var cause = new Exception("thrown with a purpose!");
    doThrow(cause).when(testStartListenerMock).onTestStart(any(TestCase.class));

    var testName = "test name";
    var packageName = "package name";

    var message = "additional message";

    var citrusRuntimeException = Assert.expectThrows(CitrusRuntimeException.class, () -> context.handleError(testName, packageName, message, cause));
    Assert.assertEquals(citrusRuntimeException.getMessage(), message);
    Assert.assertEquals(citrusRuntimeException.getCause(), cause);
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above test is failing.
If we remove
doThrow(cause).when(testStartListenerMock).onTestStart(any(TestCase.class));
this line it is passing.

I am missing something here about requirement @bbortt

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but how would you change the code, so that it does not? the invocations of testListeners#... should be handled gracefully.

}

/**
Expand Down