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.

Sign up for GitHub

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

Jump to bottom

Remove temporary directory platform-specific logic from test fixture #660

Merged
merged 2 commits into from
Feb 18, 2021
Merged

Remove temporary directory platform-specific logic from test fixture #660

merged 2 commits into from
Feb 18, 2021

Conversation

emersonknapp
Copy link
Collaborator

Depends on ros2/rcpputils#126

The remove_all functionality already existed - adding the temp dir creation logic to rcpputils to support removing all the platform-specific code.

@emersonknapp emersonknapp requested a review from a team as a code owner February 17, 2021 02:08
@emersonknapp emersonknapp requested review from zmichaels11 and prajakta-gokhale and removed request for a team February 17, 2021 02:08
@emersonknapp emersonknapp mentioned this pull request Feb 17, 2021
Merged
@emersonknapp emersonknapp force-pushed the emersonknapp/remove-tmpdir-test-logic branch from 91190da to e87f7e8 Compare February 17, 2021 02:22
Remove temporary directory platform-specific logic from test fixture
b3c7dd8 
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp emersonknapp force-pushed the emersonknapp/remove-tmpdir-test-logic branch from e87f7e8 to b3c7dd8 Compare February 17, 2021 02:22
Karsten1987
Karsten1987 reviewed Feb 17, 2021
View reviewed changes
Copy link
Collaborator

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

I am basically in favor of shifting that code over the rcpputils, I just wonder whether it makes sense to revisit the logic a little bit to adjust to a more general API.

rosbag2_test_common/include/rosbag2_test_common/temporary_directory_fixture.hpp Outdated Show resolved Hide resolved
Karsten1987
Karsten1987 approved these changes Feb 17, 2021
View reviewed changes
Copy link
Collaborator

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

I am ok if you run CI for both related PRs or merging the rcpputils one first (with individual CI then).

Update to use default tmpdir argumentO
ffca4b9 
Signed-off-by: Emerson Knapp <eknapp@amazon.com>
@emersonknapp emersonknapp force-pushed the emersonknapp/remove-tmpdir-test-logic branch from 4ccb87e to ffca4b9 Compare February 17, 2021 20:21
@emersonknapp
Copy link
Collaborator Author

Gist: https://gist.githubusercontent.com/emersonknapp/2bcaf3724861144f6265c8f1139c1150/raw/0b966dc701dcab5ec648e293d28fd03eeaa64ff3/ros2.repos
BUILD args: --packages-up-to rosbag2_test_common rosbag2_tests
TEST args: --packages-select rosbag2_test_common rosbag2_tests
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/7672

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

@emersonknapp emersonknapp merged commit e4ce24c into master Feb 18, 2021
@delete-merged-branch delete-merged-branch bot deleted the emersonknapp/remove-tmpdir-test-logic branch February 18, 2021 02:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Karsten1987 Karsten1987 Karsten1987 approved these changes

@zmichaels11 zmichaels11 Awaiting requested review from zmichaels11

@prajakta-gokhale prajakta-gokhale Awaiting requested review from prajakta-gokhale

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@emersonknapp @Karsten1987