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

Fix use of mutable default arg in tracetools_test #84

Conversation

christophebedard
Copy link
Member

@christophebedard christophebedard commented Dec 2, 2023

This issue only affects tracetools_test-based tests if multiple tests are run in the same pytest invocation (i.e., ament_add_pytest_test()). The issue is that the default list value for additional_actions is mutable. We kept appending to the same list, so we re-used Trace action objects for multiple LaunchService.run() calls, and ended up performing substitutions twice, which was raising an error.

I fixed this by changing the default value to None and then changing None to [] if needed. I also changed additional_actions to be appended instead of prepended to the list of launch actions, since the Trace action must be first; this itself would've fixed the issue too.

I also simplified the typing a bit. This additional_actions argument isn't actually used anywhere here, so it could technically be removed, but we can just keep it.

Note that test_tracetools currently has individual ament_add_pytest_test() calls for its tests, but I've bumped into this issue internally.

@christophebedard christophebedard force-pushed the christophebedard/fix-tracetools-test-mutable-default-arg branch from b495153 to 21ec7a8 Compare December 2, 2023 17:32
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me with green CI.

@christophebedard
Copy link
Member Author

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@christophebedard
Copy link
Member Author

CI looks good. Thank you for the review.

@christophebedard christophebedard merged commit 408c54b into rolling Dec 4, 2023
9 checks passed
@christophebedard christophebedard deleted the christophebedard/fix-tracetools-test-mutable-default-arg branch December 4, 2023 17:14
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

Successfully merging this pull request may close these issues.

2 participants