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

Initializing the repeat_latched option #17

Merged
merged 2 commits into from Jul 13, 2020
Merged

Initializing the repeat_latched option #17

merged 2 commits into from Jul 13, 2020

Conversation

ayrton04
Copy link

@ayrton04 ayrton04 commented Jul 2, 2020

@ayrton04
Copy link
Author

ayrton04 commented Jul 3, 2020

Added a test as well.

tappan-at-git
tappan-at-git approved these changes Jul 6, 2020
View reviewed changes
Copy link

@tappan-at-git tappan-at-git left a comment •
edited

Choose a reason for hiding this comment

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

Oh, good test. It could be worth reworking the other test in this file with the new template since

  • it could test for the case that motivated this change (repeat_latched being true without actually setting the option)
  • the existing test doesn't do anything, as far as I can tell. It publishes a latched message to a record instance, but never evaluates the bagfile. (which isn't exactly practical with record being launched from a different file)

@ayrton04
Copy link
Author

ayrton04 commented Jul 6, 2020

Yeah, how do we feel about that? If this is getting PR-ed upstream, are they going to be unhappy about the removal of the existing test?

@tappan-at-git
Copy link

Hard to say. I think if it didn't have bearing on the bug you were fixing they'd probably reject it as an unrelated change, but be open to it as a separate PR. (since the test is broken) Easiest thing might be to not change the existing test and ask explicitly whether they're open to the change.

@ayrton04
Copy link
Author

ayrton04 commented Jul 7, 2020

@tappan-at-git, sorry to do this again, but would you mind having one more look? I added a second test for the case where repeating is disabled.

tappan-at-git
tappan-at-git approved these changes Jul 7, 2020
View reviewed changes
test/test_rosbag/test/latched_pub.py
pub.publish(String("hello"))

# Wait for some time before starting the bag recording
rospy.sleep(rospy.Duration.from_sec(5.0))
Copy link

Choose a reason for hiding this comment

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

I suspect this is only part of the original test because it's waiting for the external rosbag record node to start up.

Copy link
Author

Choose a reason for hiding this comment

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

I think it's also testing latching, no? We want to confirm latching is working, so we

  • Publish
  • Wait a little while
  • Start recording

@ayrton04 ayrton04 merged commit 0dd1d92 into locus-melodic-devel Jul 13, 2020
@ayrton04 ayrton04 deleted the initialize-repeat-latched branch July 13, 2020 08:16
garyservin pushed a commit that referenced this pull request Apr 19, 2021
@ayrton04 @garyservin
Initializing the repeat_latched option (#17)
acf17c4 
* Initializing the repeat_latched option and adding test
paulbovbel pushed a commit that referenced this pull request Jan 23, 2023
@ayrton04 @paulbovbel
Initializing the repeat_latched option (#17)
b0ba107 
* Initializing the repeat_latched option and adding test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewbwhitaker matthewbwhitaker matthewbwhitaker approved these changes

@tappan-at-git tappan-at-git tappan-at-git approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
3 participants
@ayrton04 @tappan-at-git @matthewbwhitaker