Improves various points in contact sensor tests #2983
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ooctipus
requested review from
Mayankm96,
jsmith-bdai and
kellyguo11
as code owners
ooctipus
force-pushed
the
contact_sensor_test_suite_improvements
branch
from
4441f67 to
779f6c0
Compare
|
This PR requires #2842 to be merged first, because it improves upon the test written for contact positions which doesn't exist in main yet |
ooctipus
force-pushed
the
contact_sensor_test_suite_improvements
branch
from
779f6c0 to
e07cb78
Compare
ooctipus
force-pushed
the
contact_sensor_test_suite_improvements
branch
2 times, most recently
from
3819f22 to
707e54f
Compare
kellyguo11
changed the title
|
looks like this could also use a linter run |
Test Results Summary2 408 tests 1 999 ✅ 2h 15m 35s ⏱️ For more details on these errors, see this check. Results for commit f9488a7. ♻️ This comment has been updated with latest results. |
ooctipus
force-pushed
the
contact_sensor_test_suite_improvements
branch
from
289196d to
f9488a7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
the logic in original contact sensor tests was a bit convoluted,
disable_contact_processing was parameterized over many tests, even expensive ones. What is trying to test is just that creating scene will disable able it. This PR created one separate test just for that, and removed all other disable_contact_processing pytest parameterization
what suppose to be pytest parameters were hidden within a deeply nested helper function, and using multiple for loop to test each case. This can undermine the testing suites clarity when bug shows up as you don't know which hyparameter was causing the issue. I brought the for loop from deeper level to more surface level, whether or not to embed it into pytest parameter needs to confirm with original author and check for any oversight
added robust multi-envs, random contact location to stress test the collision. This tests are currently skipped due to it will fail. Need to work with sim and physx team to make sure they pass, and they will be serve for rigorous regression test.
refactored a bit of code, so all testing for contact sensors (testing air/contact time, testing contact positions, testing contact forces) can be configured nicely(avoid too many if else)
Fixes # (issue)
Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there