Bug #9910
Incorrect Error Handling for ROUTINE-LEVEL ON ERROR UNDO, THROW in FWD ABLUnit Tests
0%
Related issues
History
#1 Updated by Vladimir Tsichevski over 1 year ago
- Related to Bug #9893: LegacyErrorException is not rethrown in processForBody/processBody added
#2 Updated by Vladimir Tsichevski over 1 year ago
- reviewer Constantin Asofiei added
- reviewer deleted (
Hynek Cihlar)
In OE ABLUnit test classes, errors in tests marked with ROUTINE-LEVEL ON ERROR UNDO, THROW are correctly reported as test failures. However, in the FWD implementation, such errors trigger a message box displaying the error, and the test is incorrectly marked as passed.
The issue arises because FWD does not properly honor the ROUTINE-LEVEL ON ERROR UNDO, THROW directive. This directive should ensure errors are caught and reported as test failures, but FWD behaves as if it is absent, leading to incorrect error handling.
The following ABLUnit test class demonstrates the issue:
ROUTINE-LEVEL ON ERROR UNDO, THROW.
CLASS unittests.TestErrorHandling:
@Test.
METHOD PUBLIC VOID testInvalidDate():
/* Test an invalid date string, expecting a test failure */
DEFINE VARIABLE result AS DATE NO-UNDO.
result = DATE("invalid"). /* Should throw an error and fail the test */
END METHOD.
END CLASS.
In OE, the testInvalidDate method throws an error when parsing "invalid", causing the test to fail as expected. In FWD, a message box appears, and the test is marked as passed.
I found a relevant code fragment in the ErrorManager class that is executed during the failed error processing:
// TODO: if the current block can catch this condition OR if the current block has
// ON ERROR UNDO, THROW OR if BLOCK-LEVEL or ROUTINE-LEVEL is active for the current
// block (Depending on block type), then:
// - don't display
// - morph this into a SysError and throw it
try
{
if (manageLegacyError)
{
throwError = true;
server.recordForLegacyThrow(errmsg, num);
}
else
{
// display the error message (because of pause processing, an
// endkey, error... even stop condition can be generated here)
// it's already been logged earlier, so marking it for not logging
displayError(errmsg, false, null, true, isFromLogManager);
}
}
#3 Updated by Constantin Asofiei over 1 year ago
Eduard: please check and advise.
#4 Updated by Eduard Soltan over 1 year ago
This is the unit test result I get with trunk rev. 15859:
UI Theme successfully changed to 'material'
╷
└─ FWD Test ✔
└─ TestErrorHandling ✔
└─ testInvalidDate ✘ ** Invalid date input. (85)
Failures (1):
FWD Test:TestErrorHandling:testInvalidDate
MethodSource [className = 'com.goldencode.dataset.TestErrorHandling', methodName = 'testInvalidDate', methodParameterTypes = '']
=> org.opentest4j.AssertionFailedError: ** Invalid date input. (85)
com.goldencode.p2j.oo.lang.SysError.newInstance(SysError.java:151)
com.goldencode.dataset.TestErrorHandling.testInvalidDate(TestErrorHandling.java:43)
com.goldencode.dataset.TestErrorHandling.testInvalidDate(TestErrorHandling.java:41)
#5 Updated by Vladimir Tsichevski over 1 year ago
Eduard Soltan wrote:
This is the unit test result I get with trunk rev. 15859:
[...]
I apologize for oversimplifying the code when extracting the minimal test case from the full test class. Here is the minimal code that still reproduces the issue:
ROUTINE-LEVEL ON ERROR UNDO, THROW.
CLASS unittests.TestErrorHandling:
DEFINE FRAME myFrame.
@Test.
METHOD PUBLIC VOID testInvalidDate():
DO WITH FRAME myFrame:
DEFINE VARIABLE result AS DATE NO-UNDO.
result = DATE("invalid").
END.
END METHOD.
END CLASS.
The critical difference is using DO WITH FRAME myFrame block.
#6 Updated by Eduard Soltan over 1 year ago
Got you. I think that these tests are pretty similar.
define variable h as handle.
DEFINE VARIABLE myCustObj AS CLASS DataModelObject NO-UNDO.
def temp-table tt1
field f1 as int
field f2 as int
field f3 as int.
def var i as int.
def var j as int.
doLabel1:
do transaction:
repeat j = 1 to 4 on error undo, throw:
h = h:next-sibling.
end.
CATCH ex AS Progress.Lang.Error :
message "ERROR".
END CATCH.
end.
message "END".
In ErrorManager.recordOrThrowError we decide if the ErrorConditionException should be propagated as LegacyErrorException. For that we check:
1) if the current block has CATCH statement.
2) checks the entire block's stack for a CATCH statement if the current block is topLevel and ROUTINE-LEVEL ON ERROR UNDO, THROW. is set or just BLOCK-LEVEL ON ERROR UNDO, THROW.
3) There is another case, related to AppServers which I don't quite understand.
Only in this cases ErrorConditionException will be deferred as LegacyErrorException.
In your case DO WITH FRAME myFrame block doesn't have any CATCH statement, and it is not a topLevel block to look down the stack for CATCH statements.
This patch could be applied to enforce lookup of the whole stack, for non-topLevel blocks.
=== modified file 'src/com/goldencode/p2j/util/ErrorManager.java'
--- old/src/com/goldencode/p2j/util/ErrorManager.java 2025-04-03 20:46:09 +0000
+++ new/src/com/goldencode/p2j/util/ErrorManager.java 2025-04-16 14:56:53 +0000
@@ -1955,7 +1955,7 @@
return;
boolean silent = isSilentError();
- boolean manageLegacyError = server.mustThrowLegacyError();
+ boolean manageLegacyError = server.mustThrowLegacyError(true);
boolean mustManageError = !manageLegacyError && server.mustManageLegacyError();
boolean forceExplicitException = false;
But it is a risky change and some heavy testing should be performed.
#7 Updated by Eduard Soltan about 1 year ago
- Related to Bug #4602: fixes for OO 4GL and structured error handling added