8000 Allow unit-testing try-catch(condition) by hexagonrecursion · Pull Request #1664 · colobot/colobot · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Allow unit-testing try-catch(condition) #1664

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
Merged
merged 2 commits into from
May 19, 2024
Merged

Allow unit-testing try-catch(condition) #1664

merged 2 commits into from
May 19, 2024

Conversation

hexagonrecursion
Copy link
Contributor
@hexagonrecursion hexagonrecursion commented May 10, 2024 •
edited
Loading
  1. We want to be able to unit-test try-catch(condition)
  2. We don't want the player to have to manually single step through catch expressions when debugging code guarded by try-catch

Our CBot unit tests run in single-step mode. Our implementation of try-catch has a hack that prevents evaluation of catch conditions in single-step mode. We can't test code that we can't run.

colobot/CBot/src/CBot/CBotInstr/CBotTry.cpp

Lines 104 to 105 in e00f99f

if ( val == CBotNoErr && pile1->GetTimer() == 0 ) // mode step?
return false; // don't jump to the catch

I have removed the hack to enable unit-testing, but that caused a different problem - debugging code inside the body of try-catch became inconvenient because the player had to manually step through every catch expression after every single instruction in try body.

A solution I have found is to have the debugger automatically step over the catch expressions.

Edit: I solved this by teaching CBotTry to step-over the catch conditions. The previous implementation can be found here.

Edit: I have realized that the players may want to be able to debug the condition of catch(bool) too. New solution: skip the evaluation of the expression in catch(int) unless there was an error. This also implements CBOT enhancement proposal: untangle catch(int) and catch(bool). The previous implementation can be found here.

Here is how things work now:

try {
    //...
} catch(CBotErrZeroDiv) { // The expression `CBotErrZeroDiv` is not evaluated unless there was an exception.
} catch(CBotErrNull) { // Same
} catch(CBotErrOutArray) { // Same
} catch(x == 42) { // The expression `x == 42` is always evaluated, single stepping steps into it
}

The debugging of code guarded by catch(int) is not impacted. The debugging of code guarded by catch(int) does become less convenient, but I don't know what else I can do to improve this.

Testing

Before

git switch --detach 52804c5b4a76cb168c576543d19c7ea6a7e165b7
git restore --worktree --source HEAD^ CBot
(cd build/ && make && ./Colobot-UnitTests --gtest_filter=CBotUT.TestTryCatch)
...
Note: Google Test filter = CBotUT.TestTryCatch
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from CBotUT
[ DISABLED ] CBotUT.DISABLED_PublicClasses
[ DISABLED ] CBotUT.DISABLED_StringAsArray
[ RUN      ] CBotUT.TestTryCatch
/home/cdda/git/colobot-try-step/test/src/CBot/CBot_test.cpp:316: Failure
Failed
*** Failed test TestTryCatchCondition: CBot assertion failed
    while executing function TestTryCatchCondition (241-247)
Line 8:     ASSERT(foo == 1);

Variables:
  Block 1:
    foo = 2

/home/cdda/git/colobot-try-step/test/src/CBot/CBot_test.cpp:316: Failure
Failed
*** Failed test TestExcetionInCatchCondition: No runtime error, expected 6000


[  FAILED  ] CBotUT.TestTryCatch (13 ms)
[----------] 1 test from CBotUT (14 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (15 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] CBotUT.TestTryCatch

 1 FAILED TEST

After

git restore .
(cd build/ && make && ./Colobot-UnitTests)
...
[==========] Running 201 tests from 20 test suites.
...
[==========] 201 tests from 20 test suites ran. (389 ms total)
[  PASSED  ] 201 tests.

Before

git switch --detach 86351b8b7e24e8ee4c0513ed6c68e4c5a9b68a21
git restore --worktree --source HEAD^ CBot
(cd build/ && make && ./Colobot-UnitTests --gtest_filter=CBotUT.TestTryCatch)
...
Note: Google Test filter = CBotUT.TestTryCatch
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from CBotUT
[ DISABLED ] CBotUT.DISABLED_PublicClasses
[ DISABLED ] CBotUT.DISABLED_StringAsArray
[ RUN      ] CBotUT.TestTryCatch
/home/cdda/git/colobot-try-step/test/src/CBot/CBot_test.cpp:316: Failure
Failed
*** Failed test ErrorCodeIsNotEvaluatedUnlessThereIsAnException: RUNTIME ERROR - 6000
    at unknown location 181-182
Line 4:     } catch(1/0) { // This should not throw because it's never evaluated

Variables:

/home/cdda/git/colobot-try-step/test/src/CBot/CBot_test.cpp:316: Failure
Failed
*** Failed test CatchZeroIsNotCatchTrue: CBot assertion failed
    while executing function CatchZeroIsNotCatchTrue (303-309)
Line 9:     ASSERT(result == 1);

Variables:
  Block 1:
    result = 2

[  FAILED  ] CBotUT.TestTryCatch (21 ms)
[----------] 1 test from CBotUT (21 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (22 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] CBotUT.TestTryCatch

 1 FAILED TEST

After

git restore .
(cd build/ && make && ./Colobot-UnitTests)
...
[==========] Running 201 tests from 20 test suites.
...
[==========] 201 tests from 20 test suites ran. (366 ms total)
[  PASSED  ] 201 tests.

Valgrind

@hexagonrecursion hexagonrecursion mentioned this pull request May 10, 2024
Open
@hexagonrecursion

This comment was marked as duplicate.

Sign in to view
@hexagonrecursion hexagonrecursion marked this pull request as draft May 11, 2024 05:48
@hexagonrecursion
Copy link
Contributor Author
hexagonrecursion commented May 11, 2024 •
edited
Loading

I have changed my mind about how to implement this. Will update this pull request soon. Edit: I pushed the new solution. Leaving this pull as a draft until tomorrow in case I have more thoughts about this in the meantime.

@hexagonrecursion hexagonrecursion marked this pull request as ready for review May 12, 2024 14:53
@hexagonrecursion
Copy link
Contributor Author

I solved this by teaching CBotTry to step-over the catch conditions

I think I found a better alternative

@hexagonrecursion hexagonrecursion marked this pull request as draft May 13, 2024 03:08
hexagon-recursion added 2 commits May 16, 2024 09:05
Allow unit-testing try-catch(condition)
52804c5 
Our CBot unit tests run in single-step mode.
Try-catch had a hack that prevented evaluation of catch conditions in single-step mode.

I have removed the hack to enable unit-testing, but that caused a different problem:
debugging code inside the body of try-catch became inconvenient because
the player had to manually step over every catch expression after every single instruction in try body.

This will be addressed in the next commit
Don't evaluate catch(int) unless there was an error
86351b8 
Also implements colobot#1629
@hexagonrecursion
Copy link
Contributor Author

Leaving this as a draft for a couple more days in case I have more ideas.

@hexagonrecursion hexagonrecursion marked this pull request as ready for review May 19, 2024 04:00
@tomaszkax86 tomaszkax86 merged commit 3dc72b1 into colobot:dev May 19, 2024
9 of 10 checks passed
@hexagonrecursion hexagonrecursion deleted the try-step branch May 22, 2024 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomaszkax86 tomaszkax86 tomaszkax86 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hexagonrecursion @tomaszkax86
0