Refactored test code smells #397
Conversation
…eliminate eager tests
…ion to eliminate eager tests
…tion and improve understandability.
…in Unit Tests for EntityAction and EntityActionMap. Removes eagerness, assertion roulette and indirect testing.
…parate unit tests, eliminating eager tests and assertion roulette.
…der to reduce code duplication.
… code duplication.
…eliminate eager tests
…t tests for the overloads, removing eager tests and reducing duplication
…imensionalArrayToList
| // TODO: target collision box wrongfully detects an intersection with blocking collision box due to floating point precision miscalculation | ||
| // within method GeometricUtilities.intersects - intersection is marginally positive in case of left movement, but marginally negative in right movement | ||
| // Game.physics().move(ent, 315, MOVE_10X10Y_DISTANCE); | ||
| Game.physics().move(ent, 315, MOVE_10X10Y_DISTANCE-(1e-14)); |
There was a problem hiding this comment.
@nightm4re94 @Gamebuster19901 During development we found this issue with floating point precision during calculating collisions and intersections.
Basically the test will fail if I provide the constant as it is, but it will pass if I subtract a marginal value. The issue is that due to the floating point precision it miscalculates an intersection to exist where there should not be any...
How should we handle this problem?
There was a problem hiding this comment.
is 1e-14 the order of magnitude of the difference in this example?
I think it's odd that a number as small as 0.00000000000001 has an effect on the floating number equality assertion, as we've defined the tolerance threshold EPSILON = 1e-6 = 0.000001.
There was a problem hiding this comment.
Exactly my point. However, the problem is not the threshold of EPSILON (since it is only relevant for the assertion), but lies in the underlying computations within GeometricUtilities.intersects(). The small delta seems to trigger a floating point change that results in finding an intersection between the two CollisionBoxes when they really just touch instead of intersect. As a result, the movement is handled differently and the target position is off the expectation
There was a problem hiding this comment.
ah, I see! Thanks for clarifying!
There was a problem hiding this comment.
to come back to the original question: how should we proceed with this?
Do you guys work with the Github Issues as backlog (i.e., should we open an issue and ignore the problem for now in this PR), or should we include a potentially larger refactoring in this PR?
There was a problem hiding this comment.
I think this would best be tracked separately - feel free to open an issue.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I think it would probably make sense to integrate #398 first, then it is simpler to fix the conflicts of this one (both changed EntityTests) |
…to consider changes of PR#396 and PR#398
|
Resolved the conflicts for ArrayUtilitiesTests, EntityTests and GeometricUtilitiesTests. |
I locally switched the methods to use |
Not sure what you mean, the physics already only uses Basically |
Okay, then I will ignore it for now. I was mostly irritated that no tests would fail when exchanging methods (if you say it should return something different). But this is most likely due to not properly covering this method in general, which is what we are currently trying to fix ^^ point noted, will probably try to address that in the upcoming changes we plan 👍 => PR should now be free of open issues and conflicts |
* refactor: move tests to correct package * refactor: fix test param supply docs links * feat: specifically treat line intersection and add tests * refactor: use GeoUtils:intersects for rectangles * Implemented unit test for PhysicsEngine::raycast(Line2D, Collision, ICollisionEntity) * Implemented unit test for PhysicsEngine constructor * Implemented further unit tests for PhysicsEngine::raycast() overloads * Implemented unit tests for PhysicsEngine::raycast overloads * Implemented additional unit tests for GuiComponent::getTweenValues and setTweenValues * Implemented unit test for GuiComponent::getTextToRender * Implemented unit test for GuiComponent::getShape * Implemented unit tests two overloads of SpeechBubble::create * Added null assertion to PhysicsEngineTests::testRaycastCollision * Refactor FileUtilitiesTests code * Slight adjustment of PhysicsEngineTests * Improved variable names in PhysicsEngineTests * Add test cases for deleteDir and findFilesByExtension * Remove unused imports * fix: fix flaky test checking for array order in unordered array * refactor: restructure test file * Refactor test cases for FileUtilities * refactor: integrate ParticleTests in correct package Co-authored-by: Niels Kübler <niels.kuebler@uzh.ch> Co-authored-by: ddreimane <dreimane.dace@gmail.com>
Refactored test code smells to improve test code quality.