Skip to content
New issue

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

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

Already on GitHub? # to your account

Unit test friendly folder structure for Python examples #237

Merged
merged 6 commits into from
Jan 13, 2025

Conversation

sea-bass
Copy link
Collaborator

@sea-bass sea-bass commented Jan 11, 2025

Attempts to address #233 by moving files around so that there is no module that could be wrongly imported in the source folder of the Python example package.

Turns out the key is to just not have an __init__.py file in the module in the source folder, but instead to generate it only into install space.

... and just as before, if you install with --symlink-install, the __init__.py file will be thrown into the source folder anyhow.

@sea-bass sea-bass force-pushed the unit-test-friendly-structure branch from 8d1ee44 to 9b04476 Compare January 11, 2025 03:47
@@ -55,6 +55,7 @@ def generate_parameter_module(module_name, yaml_file, validation_module=''):
install_dir = os.path.join(
colcon_ws,
'install',
pkg_name,
Copy link
Collaborator Author

@sea-bass sea-bass Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be fixed in #236, but needed for this PR to work properly

@sea-bass sea-bass changed the title Unit test friendly structure Unit test friendly folder structure for Python examples Jan 11, 2025
@sea-bass sea-bass force-pushed the unit-test-friendly-structure branch 2 times, most recently from 8e14998 to 52e7c43 Compare January 11, 2025 03:56
@sea-bass sea-bass force-pushed the unit-test-friendly-structure branch from 52e7c43 to 37ad69e Compare January 11, 2025 03:57
@sea-bass sea-bass force-pushed the unit-test-friendly-structure branch from 2eb5336 to b0aee7c Compare January 11, 2025 04:09
@sea-bass sea-bass marked this pull request as ready for review January 11, 2025 04:09
@sea-bass sea-bass requested a review from JafarAbdi January 11, 2025 04:12
@sea-bass sea-bass mentioned this pull request Jan 11, 2025
3 tasks
Copy link
Collaborator

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I don't have much experience with python install stuff to review that 🤷‍♂️

@sea-bass
Copy link
Collaborator Author

Sorry, I don't have much experience with python install stuff to review that 🤷‍♂️

No worries, I'm hoping the handful of people engaged in the associated issue will get a chance to test here. I just can't officially tag most of them as reviewers as they are not repo collaborators.

Copy link
Contributor

@MarqRazz MarqRazz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally with my own package and this fixes my testing issues.

Copy link

@bmagyar bmagyar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much needed, thank you!

@sea-bass
Copy link
Collaborator Author

sea-bass commented Jan 13, 2025

@christophfroehlich could you give me a green approve on behalf of Marq and Bence, so I can merge this and cut the release? 🙏🏻

@sea-bass sea-bass merged commit 739dfcb into main Jan 13, 2025
7 checks passed
@sea-bass sea-bass deleted the unit-test-friendly-structure branch January 13, 2025 17:42
@imcelroy
Copy link

imcelroy commented Jan 14, 2025

@sea-bass I'm sorry I'm late, I see this has already been merged!

I left a comment on the issue #233 (comment). I might be missing something, but removing the __init__.py doesn't seem to be standard practice. The python documentation says to put it in the source, ROS examples and the ROS CLI (ros2 pkg create) will put it in the source folder (here), etc, so I'm not sure this is a good long-term solution.

Additionally, it's true that package does successfully build, but I now have the following stderr, which also suggests that this isn't standard practice:

--- stderr: generate_parameter_module_example                                                                                     
package init file 'generate_parameter_module_example/__init__.py' not found (or not a regular file)
---
Finished <<< generate_parameter_module_example [2.32s]

In any case, thank you very much for your reactivity and work on this issue!

@sea-bass
Copy link
Collaborator Author

Oh, interesting. I had not seen that warning for some reason. That is... not ideal.

Yeah, it's a problem because this isn't a "regular" Python module where all the code is available in the source folder.

I guess if we go back to the state before this PR, the only bulletproof way to test these generared modules is to do it from another package.

Anyways, would suggest we keep iterating and if we find something good we can release again. Thanks!

@imcelroy
Copy link

imcelroy commented Jan 14, 2025

Oh, interesting. I had not seen that warning for some reason. That is... not ideal.

Yeah, it's a problem because this isn't a "regular" Python module where all the code is available in the source folder.

I guess if we go back to the state before this PR, the only bulletproof way to test these generared modules is to do it from another package.

Anyways, would suggest we keep iterating and if we find something good we can release again. Thanks!

@sea-bass I agree with iterating. I only wanted to point out that this will break existing unit tests for users that are trying to adopt generate_parameter_library into an existing ROS2 python package (like myself) after having followed ROS guidelines.

I think part of the issue is that the generated parameter module is treated as something "compiled" like in C++. However, from what I understand that isn't how Python works, which is why everything is in the source folder.

As a potential solution to the source control issue that you mentioned here: #233 (comment), the user could add it to their .gitignore.

Thank you again for your help!

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

Successfully merging this pull request may close these issues.

6 participants