Convert axisAlignedCollision tasts to Catch2 and improve docs
#15427
Conversation
Zughy
added
Maintenance
|
#15408 adds additional collision unit tests. |
|
@kno10 That looks great, thank you! If that lands before this one, I can rebase this over some weekend thereafter. |
JosiahWI
force-pushed
the
refactor/catch2-axis-aligned-collision
branch
from
20d30ae to
702e7dd
Compare
|
Rebased on master. Ready for review. |
src/unittest/test_collision.cpp
Outdated
| SECTION("Given a 0.5x2x1 cube translated by -2 units on the x-axis " | ||
| "and -1.5 units on the y-axis, " | ||
| "when it moves 0.5 units per step in the +x direction " | ||
| "and 0.1 units per step in the +y direction for 3 steps, " | ||
| "then it should collide on the X axis within epsilon of 3 steps.") |
There was a problem hiding this comment.
having throughout but hard to read textual description that duplicates the test contents looks pretty useless to me
There was a problem hiding this comment.
Yeah, this is pretty bad. I will think about how to describe the intent of the test better.
There was a problem hiding this comment.
I took a shot at it. Is there anything else I should do to improve them?
| @@ -16,15 +20,13 @@ class TestCollision : public TestBase { | |||
|
|
|||
| void runTests(IGameDef *gamedef); | |||
|
|
|||
| void testAxisAlignedCollision(); | |||
| void testCollisionMoveSimple(IGameDef *gamedef); | |||
There was a problem hiding this comment.
I think we shouldn't do partial migrations so that catch and regular tests are mixed in a single file.
Or is there no solution to pass gamedef to tests yet?
There was a problem hiding this comment.
There isn't, yet, but maybe this is the right time to make that solution.
There was a problem hiding this comment.
I will work on a patch for that, and then we can decide whether to do that here or separately.
sfan5
added
the
Action / change needed
* Use `float` overload of `std::fabs` to not necessitate promotion. * Word test descriptions to describe intent instead of method.
JosiahWI
force-pushed
the
refactor/catch2-axis-aligned-collision
branch
from
0e3dbb0 to
cd5fcd5
Compare
This is just cleanup. The content of the unit tests should not be changed, although one redundant test was removed. It looked like the author copy-pasted a test, intending to change the test parameters, but forgot to change them. Since I don't know what the author intended to change, I took it back out. I have not converted the
collisionMoveSimpletests that were recently added, and I do not have time to in the near future.I have also reworded the docstring of the function under test to clarify that
dtimeis an inout parameter. The original wording does not seem to mention its use as an input to the function. This information came from @appgurueu, and I observed it to be correct based on the unit test behavior; I have not read the code of the function under test.To do
Ready for Review.
How to test
luanti --run-unittests --test-module "[collision]"