Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add missing test test_fixed_foot_detector.py to Python tests #658

Merged
merged 2 commits into from
Apr 28, 2023

Conversation

traversaro
Copy link
Collaborator

@traversaro traversaro commented Apr 27, 2023

There was an unexpected test failure in conda-forge/bipedal-locomotion-framework-feedstock#25, and it turned out a test was not run in the CI.

@traversaro
Copy link
Collaborator Author

I also tried to fix the test, with this modification:

+++ b/bindings/python/Contacts/tests/test_fixed_foot_detector.py
@@ -5,13 +5,12 @@ pytestmark = pytest.mark.contact_detectors
 import bipedal_locomotion_framework.bindings as blf
 import manifpy as manif
 import numpy as np
-
+from datetime import timedelta

 def test_fixed_foot_detector():
-    dt = 0.01
-
     parameters_handler = blf.parameters_handler.StdParametersHandler()
-    parameters_handler.set_parameter_float("sampling_time", dt)
+    dt = timedelta(seconds=0.01)
+    parameters_handler.set_parameter_datetime("sampling_time", dt)

     detector = blf.contacts.FixedFootDetector()
     assert (detector.initialize(parameters_handler))
@@ -65,7 +64,7 @@ def test_fixed_foot_detector():
     phase_list = blf.contacts.ContactPhaseList()
     phase_list.set_lists(contact_lists=contact_list_map)

-    assert (detector.set_contact_phase_list(phase_list))
+    detector.set_contact_phase_list(phase_list)

     # Retrieve initial fixed foot
     fixed_foot = detector.get_fixed_foot()

but now I am hitting the problem:

        phase_list = blf.contacts.ContactPhaseList()
        phase_list.set_lists(contact_lists=contact_list_map)

        detector.set_contact_phase_list(phase_list)

        # Retrieve initial fixed foot
        fixed_foot = detector.get_fixed_foot()

>       assert (fixed_foot.name == "l_sole")
E       AssertionError: assert 'Contact' == 'l_sole'
E         - l_sole
E         + Contact

python/Contacts/tests/test_fixed_foot_detector.py:72: AssertionError
---------------------------------------------------------------------- Captured stderr call -----------------------------------------------------------------------
[ERROR] [2023-04-27 23:01:53.522] [thread: 7556] [blf] [FixedFootDetector::getFixedFoot] Unable to find the fixed foot. This should never happen.

@GiulioRomualdi
Copy link
Member

Let me try to give it a look

@traversaro
Copy link
Collaborator Author

I guess it is a matter of adding timedelta(seconds=<>) in the right places.

@traversaro
Copy link
Collaborator Author

Small OT: there are also a lot of warnings like:

PytestUnknownMarkWarning: Unknown pytest.mark.parameters_handler - is this a typo?  You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/stable/how-to/mark.html
    pytestmark = pytest.mark.parameters_handler

not sure if this is intentional or not.

@traversaro
Copy link
Collaborator Author

There are also a lot of Ubuntu failure like:

TypeError: Unregistered type : manif::SE3<double>

this is because the compilers meta package installs a gcc version different from the one that is used in building the manifpy recipe, I will try to fix that at the conda-forge level.

@GiulioRomualdi
Copy link
Member

Hi tried to replicate the failure but everything seems working fine on my PC.
All the dependencies are installed with apt and superbuild compiled from src

I've tested this commit 3628840 (The last of this PR)

@traversaro
Copy link
Collaborator Author

There are also a lot of Ubuntu failure like:

TypeError: Unregistered type : manif::SE3<double>

this is because the compilers meta package installs a gcc version different from the one that is used in building the manifpy recipe, I will try to fix that at the conda-forge level.

This will be fixed by conda-forge/compilers-feedstock#57 .

@GiulioRomualdi
Copy link
Member

Hi tried to replicate the failure but everything seems working fine on my PC. All the dependencies are installed with apt and superbuild compiled from src

I've tested this commit 3628840 (The last of this PR)

Ok that's strange the test is not called via ctest probably because I've a dirty build folder 🤔

@GiulioRomualdi
Copy link
Member

GiulioRomualdi commented Apr 28, 2023

The test #658 (comment) was failing because of a regression caused by 95a29b4.
I fixed it in b91fb10

@traversaro
Copy link
Collaborator Author

Thanks, great! If the Windows Conda CI passes I think we can merge, the other CIs will work as soon as conda-forge/compilers-feedstock#57 is merged, but I guess we need anyhow to wait at least for the US morning.

@GiulioRomualdi
Copy link
Member

Shall we release a new version?

@traversaro
Copy link
Collaborator Author

traversaro commented Apr 28, 2023

Shall we release a new version?

For conda-forge it is not necessary, I just skipped the broken test there, but there was no actual bug in the non-test code.

@GiulioRomualdi
Copy link
Member

Since windows conda ci is passing I'm going to merge the PR as @traversaro suggested in #658 (comment)

@GiulioRomualdi GiulioRomualdi merged commit a2e76a8 into master Apr 28, 2023
@GiulioRomualdi GiulioRomualdi deleted the addmissingtests branch April 28, 2023 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants