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

Regression - Python tests no longer discovered if using new setuptools. #4727

Open
Ryanf55 opened this issue Sep 7, 2024 · 2 comments
Open

Comments

@Ryanf55
Copy link
Contributor

Ryanf55 commented Sep 7, 2024

Description

My tests for an ament_python package are no longer being recognized. I went through a basic package creation, then followed
the python testing tutorial.

When you run colcon test, the tests no longer run. The test file for 2+2=5 is no longer run and colcon test passes.

Details

  • OS: Ubuntu 22
  • ROS Distribution: humble
  • My environment had python packages installed the user environment, including setuptools 73.0.1

Logs

ros2 pkg create --build-type ament_python my_test_python
colcon build --packages-up-to my_test_python
Starting >>> my_test_python
/home/ryan/.local/lib/python3.10/site-packages/setuptools/_distutils/dist.py:268: UserWarning: Unknown distribution option: 'tests_require'
 warnings.warn(msg)
--- stderr: my_test_python                   
/home/ryan/.local/lib/python3.10/site-packages/setuptools/_distutils/dist.py:268: UserWarning: Unknown distribution option: 'tests_require'
 warnings.warn(msg)
---
Finished <<< my_test_python [1.01s]

Summary: 1 package finished [1.53s]

Implications

CI for ArduPIlot is no longer running our tests. This has allowed regressions to crop in our DDS interface. This is a huge issue for us.
This linked issue is here: ArduPilot/ardupilot#27925

Should we have a more clear warning that if you don't use the OS supplied setuptools, your tests won't be detected?
I'd ideally want something to throw an error if you use the new setuptools with colcon.

@clalancette
Copy link
Contributor

Should we have a more clear warning that if you don't use the OS supplied setuptools, your tests won't be detected?

The problem is that that advice is not true in general, just sometimes. For instance, you were using the non-OS-supplied setuptools for quite a while before it broke. Further, colcon is a generic tool, not just for ROS, and thus whether things work or not depends on both the environment and the package.

That said, I could imagine adding an option to colcon to tell it to "fail" the tests if it found zero tests. We couldn't make this the default, since it is perfectly legitimate for a package to have no tests, but it might be a useful option for CI pipelines. That's just one thought, there might be others. Pinging @cottsay for additional thoughts.

@cottsay
Copy link
Member

cottsay commented Sep 26, 2024

Hi, thanks for the report. I have a couple of thoughts.

  1. We are overdue to update the referenced documentation. The tests_require option was never official, and the modern practice (which is also not official) is to use an extras_require named test, tests, or testing. The change in setuptools that caused the change in behavior is that tests_require was pretty much dropped, and the mechanism we're using to read the python metadata no longer reports that field.
  2. In regard to how to behave when there are no tests, we're at the mercy of the underlying testing tool here. In fact, colcon doesn't have any idea how many tests there are, it just invokes the tool and checks the return code. There isn't any difference between a test run that actually ran no tests and a test run which didn't produce any JUnit/XUnit/etc result files for colcon test-result to find.

In this case, colcon no longer knew what Python test tool to use for this package because tests_require stopped working, so it defaulted to unittest instead of pytest. It's possible to write tests which can be invoked by either of those testing tools, but evidently these tests aren't able to be run by unttest.

Try adding this in place of tests_require:

    'extras_require': {
        'test': [
            'pytest',
        ],
    },

If that works as expected, we should update the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants