CATROID-799 Add Catrobat language test for SetGravityForAllActorsAndObjects brick #3848
Conversation
There was a problem hiding this comment.
Please don't use Wait-functions, they tend to not work if the wait duration is to small on some slower devices and if the wait duration is to long the test gets unnecessarily too long.
A possible solution is to use Event-Bricks that wait for a condition to be true.
Additionaly the Finish Brick here might report "Success" before all asserts are checked.
This can be fixed if the finish brick is triggered when all assert are finished, if the finish brick is never reached the test will fail on time-out
In the attached version I provided a possible solution, (not the most elegant one, as the wait condition is also the same as the assert)
There was a problem hiding this comment.
Thanks.
However, the tests contain race conditions, where the outcome is implementation-dependent. For instance, in object Apple, the first script at the beginning executes the "Set your motion type" brick. Since the gravity vector by default is pointing down, the Apple starts to fall. Same for Apple2 as soon as the "Set your motion type" brick is executed in its first script. But you check only two bricks later, and for Apple2 also some bricks later, and not synchronized in a secure way, that the y velocity is still 0. This may not work on slow emulators, since the objects will have started to move already after the two bricks before the assert bricks, and thus they may have negative y velocities already. Also, the synchronization for Apple2 for the gravity set to 0 is not ensured, since there's no guarantee in which order the "When scene starts" scripts of the two objects are executed, and which bricks in the two objects are executed first in an interlaced, multithreaded order.
Also, since we have an Assert equal brick, there's no need to insert the equality test into the first parameter fields.
The synchronization via counting up the "velocity tested" variable is not easy to understand, and this can be done much simpler and clearer.
There are many ways how to fix this. Please think it through clearly once more and resubmit.
There was a problem hiding this comment.
Thanks, much better. However, there could still be race-condition problems making the test flaky, now on both slow and fast emulators (i.e., the latter mainly relevant hopefully in the future ;-) ), as follows:
For the Apple objects, in the first scripts, there is no brick between setting the motion type and the assertion. On fast emulators, the velocity thus could be 0 even with a non-zero gravity in the y direction. Therefore, please insert a "Wait 0.1 seconds" brick in between, at the point indicated with a "1" in the screenshot below. Then please test first that if the gravity has not been reset in the background, the assert in the first scripts would now fail, and document this in a reply in the comment section of the PR. If the test now doesn't fail in this case, increase the waiting time, e.g., to 0.2 or even larger, till the assert fails reliably. Then of course change the gravity back to 0 in the background, so that the assert works fine again.
For the second scripts of the Apple objects, there still is the problem on slow emulators that the velocity may not yet be negative, which could make the test flaky. Therefore, again insert a wait brick, now at position "2" indicated in the screenshot below, with the same time as in the case of the first scripts.
cc8d1f9 to
b210e8d
Compare
|
Thanks, I tested it and the asserts in the first scripts fail reliably with a "Wait 0.1 seconds" when the gravity has not been set to 0. |
|
Perfect, thanks a lot! |
Added a new Catrobat language test for the SetGravityForAllActorsAndObjects brick.
https://jira.catrob.at/browse/CATROID-799
Your checklist for this pull request
Please review the contributing guidelines and wiki pages of this repository.