Closed Garados007 closed 1 year ago
@t-b @MichaelHuth Ready for review.
I found a small bug in the results output while checking #54. I have included the fix and updated the documentation for this.
I will add #54 to the close list of this PR as the main topic (number of user hook abort errors inside JUnit) is solved here.
Review:
a748b2a9 (Refactor test reporting results controls, 2023-01-12)
99f2fdb1 (Rename unit-testing-hooks to unit-testing-hooks-proto, 2023-01-12) 5017e0de (Refactor Hooks into own file, 2023-01-12)
Looks all sensible.
729a8c38 (Hooks: Refactor ExecuteHooks to be cleaner, 2023-01-12)
+/// @returns 1 if user hook succeed without an error and 0 if an error happened
result
variable, which unfortunately is needed here, to 1. The default for a result variable should always be the error state.From ExecuteHooks:
if(ExecuteUserHook(name, userHook, procWin, HOOK_LEVEL_TEST_CASE))
AfterTestCaseUserHook(name, param)
endif
Two questions:
Why do we do that even for skipped test cases?
Why do we call AfterTestCaseUserHook
only on sucessfull hook invocation? We seem to never do that in other cases?
Can we have a test which verifies that the current behaviour for skipped test cases and the test case hooks stays constant?
7160539b (Treat user hooks as test cases for the results, 2023-01-13)
FuncRefInfo
instead of parsing the name.ec24ad06 (Refactor: Remove logTestCase parameter from EvaluateRTE, 2023-01-16)
Good call!
I will add https://github.com/byte-physics/igor-unit-testing-framework/issues/54 to the close list of this PR as the main topic (number of user hook abort errors inside JUnit) is solved here.
I fail to see who this fixes #54. Please explain that in a commit message.
Why do we do that even for skipped test cases?
That's an interesting bug. While performing all refactor processes I tried to keep old behavior as much as possible so I haven't changed this logic. This is a bug that is also in main.
I'll create an issue for this. --> #384
Why do we call
AfterTestCaseUserHook
only on sucessfull hook invocation? We seem to never do that in other cases?
Same as above. In main AfterTestCaseUserHook
is skipped when the user hook exists with an abort or RTE.
I'll create an issue for this. --> #385
Can we have a test which verifies that the current behaviour for skipped test cases and the test case hooks stays constant?
I need more clarification. The current behavior with skipped tests is that TEST_CASE_BEGIN_OVERRIDE
is not executed but TEST_CASE_END_OVERRIDE
is. You can use this as simple test:
#include "unit-testing"
Function TEST_CASE_BEGIN_OVERRIDE(string name)
printf "hook begin: %s\n", name
End
Function TEST_CASE_END_OVERRIDE(string name)
printf "hook end: %s\n", name
End
// UTF_SKIP
Function TestCase()
print "test case"
PASS()
End
// cac6bdf (Merge pull request #348 from byte-physics/mb/fix/329-print-routines, 2023-01-16)
•RunTest("Procedure")
Start of test "Unnamed"
Entering test suite "Procedure"
hook end: TestCase
Finished with no errors
Leaving test suite "Procedure"
Test finished with no errors
End of test "Unnamed"
// dea03ce (Refactor: Remove logTestCase parameter from EvaluateRTE, 2023-01-16)
•RunTest("Procedure")
Start of test "Unnamed"
Entering test suite "Procedure"
hook end: TestCase
Finished with no errors
Leaving test suite "Procedure"
Test finished with no errors
End of test "Unnamed"
Or did you have something else in mind?
I fail to see who this fixes https://github.com/byte-physics/igor-unit-testing-framework/issues/54. Please explain that in a commit message.
Fixed.
For determining if a funcref is initialized you can use FuncRefInfo instead of parsing the name.
Good idea. I tried it but doesn't work. It's for both cases the same output:
NAME:TEST_CASE_END_OVERRIDE;ISPROTO:0;ISXFUNC:0;
NAME:TEST_SUITE_END;ISPROTO:0;ISXFUNC:0;
I keep the name parsing.
Or did you have something else in mind?
I think the test belongs then to PR for #384. Thanks for for managing the issues.
For determining if a funcref is initialized you can use FuncRefInfo instead of parsing the name.
Good idea. I tried it but doesn't work. It's for both cases the same output:
NAME:TEST_CASE_END_OVERRIDE;ISPROTO:0;ISXFUNC:0; NAME:TEST_SUITE_END;ISPROTO:0;ISXFUNC:0;
Got it. I guess SetDefaultHooks is reponsible for that. Let's keep your approach.
Review round 2:
ExecuteUserHook: The documentation says
The return is still 1 as if no error happened.
but that is wrong as 0 is now success.
Rest looks good!
Fixed.
Assertions in User Hooks are now working again and with a clean output in JUnit or TAP. At the same time the code is a bit refactored to allow these modification and make it more cleaner and easier to maintain. During these steps the solution for Issue #344 is included as it helped to implement #359.
Close #344 Close #359