Conversation
Greptile SummaryThis PR refactors the test infrastructure by consolidating 13 pytest markers down to 3 ( Key changes:
The refactor makes the test suite more maintainable by reducing complexity and providing clearer categorization between fast/slow tests. Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 3e10cd5 |
8ac5305 to
f5f6f03
Compare
3e10cd5 to
70d79b5
Compare
There was a problem hiding this comment.
I love the color red when it comes to PR's. My only questions are:
- is cuda still fine? (E.g. non cuda systems just don't add the cuda flag, that's why we don't need a has_cuda check)
- I'm a bit unsure of our policy for
.stop/close/shutdown/unsub. It feels like there should be a consistent naming system. Like stop is always stop-all or some similar rule.
| nav.stop() | ||
| nav.stop_rpc_client() | ||
| robot.stop_rpc_client() | ||
| dimos.close_all() |
There was a problem hiding this comment.
Is this typical/recommended over dimos.shutdown?
There was a problem hiding this comment.
Yes, dimos.close_all does a few other things, but we won't be using Dask for long as I'm removing it.
There was a problem hiding this comment.
I guess we can have whatever our forking coordinator is - conform to Resource so stop() recursively goes through active modules/processes?
|
|
||
| @rpc | ||
| def stop(self) -> None: | ||
| def unsub_all(self) -> None: |
There was a problem hiding this comment.
Seems kinda nice for stop to be a universal method that includes stuff like unsub_all.
There was a problem hiding this comment.
I agree. But this test module defined stop whilst not actually calling super().stop() and then calling dimos.close_all(). It felt easier at the time to just rename but I should probably fix it the right way. 😅
There are no CUDA tests left. The Image CUDA stuff was removed a while ago. And the Metric3D thing was removed recently. It might make sense to add it again later, but not needed anymore now. |
|
|
||
| ci-complete: | ||
| needs: [check-changes, ros, python, ros-python, dev, ros-dev, run-tests, run-heavy-tests, run-lcm-tests, run-integration-tests, run-ros-tests, run-mypy] | ||
| needs: [check-changes, ros, python, ros-python, dev, ros-dev, run-ros-tests, run-mypy] |
There was a problem hiding this comment.
just to know what the plan was with ros/non ros parallel tests - I planned to treat ros build as a separate OS, since it does heavy intervention into the OS itslef, and wanted to make sure that unguarded ros imports don't crash non-ros machines, so potentially we want to re-introduce
There was a problem hiding this comment.
It makes sense to test that, but maybe not on every PR? Maybe we can have a periodic runner which runs all the tests on a variety of environments like Ubuntu 22.04/24.04, with ROS/without ROS, MacOS, etc.
leshy
left a comment
There was a problem hiding this comment.
This is so amazing, thanks Paul
|
Paul is codebase Jesus, suffering for our sins |
70d79b5 to
e65a531
Compare
Problem
Our testing setup is quite complicated.
Closes DIM-559
Solution
tool,slow(replacement forheavy/integration/e2e), andmujoco.pytesttests.neverendingandmoduletests.skipif_in_ci,skipif_no_openaito lessen duplication.Breaking Changes
None
How to Test
Run the commands from https://github.com/dimensionalOS/dimos/blob/paul/fix/testing-docs-2/docs/development/testing.md .
Contributor License Agreement