Skip to content

Update quoted comments in the test.#540

Merged
tfoote merged 3 commits intomasterfrom
tfoote/fix_tests_for_changed_comment
Jun 23, 2020
Merged

Update quoted comments in the test.#540
tfoote merged 3 commits intomasterfrom
tfoote/fix_tests_for_changed_comment

Conversation

@tfoote
Copy link
Copy Markdown
Contributor

@tfoote tfoote commented Jun 23, 2020 •
edited
Loading

Followup to ros2/rcl_interfaces#103 that collided with #527

@audrow This test is quite fragile and depends on very specific elements from a package that's only an indirect dependency. For robustness it might be worth specifically having controlled test messages that can be versioned with the code to avoid the fragility of relying on things like comments in other packages to remain static. I don't think that there's a significant benefit/purpose to test builtin_interfaces/test_msgs specifically vs any message you define with the expected structure. Otherwise this package will be required to be released in lockstep with any change to the message definitions in builtin_interfaces or test_msgs.

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

@tfoote
Update quoted comments in the test.
58abfd7 
Signed-off-by: Tully Foote <tfoote@osrfoundation.org>
@tfoote tfoote requested a review from audrow June 23, 2020 01:54
@tfoote
fix quotes
c3943f7 
Signed-off-by: Tully Foote <tfoote@osrfoundation.org>
@audrow
Fix expected lines in test show all comments
77f9c38 
Signed-off-by: Audrow <audrow.nash@gmail.com>
@audrow
Copy link
Copy Markdown
Member

audrow commented Jun 23, 2020 •
edited by dirk-thomas
Loading

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

@jacobperron
Copy link
Copy Markdown
Member

jacobperron commented Jun 23, 2020

I don't think that there's a significant benefit/purpose to test builtin_interfaces/test_msgs specifically vs any message you define with the expected structure.

I agree. I think test_msgs was originally used since it is our go-to interface package for use in unit tests in a lot of packages. We inherently have the issue that changes to messages in test_msgs may require changes in a lot of downstream packages (e.g. system_tests), but it's a trade-off as we avoid maintaining separate interface packages for each package we want to test. For ros2interface in particular, it may make sense to maintain a separate interface package since the tests are especially sensitive towards the exact contents of interface definitions.

jacobperron
jacobperron approved these changes Jun 23, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@jacobperron jacobperron left a comment

Choose a reason for hiding this comment

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

LGTM pending CI

audrow
audrow approved these changes Jun 23, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@audrow audrow left a comment

Choose a reason for hiding this comment

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

Looks good, pending CI

@tfoote tfoote merged commit 95c6357 into master Jun 23, 2020
@delete-merged-branch delete-merged-branch bot deleted the tfoote/fix_tests_for_changed_comment branch June 23, 2020 20:34
@audrow audrow mentioned this pull request Jun 23, 2020
Closed
@audrow
Copy link
Copy Markdown
Member

audrow commented Jun 23, 2020

For ros2interface in particular, it may make sense to maintain a separate interface package since the tests are especially sensitive towards the exact contents of interface definitions.

I agree. I made an issue #541 and assigned it to myself.

@dirk-thomas dirk-thomas mentioned this pull request Jul 6, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jacobperron jacobperron jacobperron approved these changes

@audrow audrow audrow approved these changes

Assignees

@tfoote tfoote

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tfoote @audrow @jacobperron @dirk-thomas