-
Notifications
You must be signed in to change notification settings - Fork 96
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
[go/cli] Fix uint64 bug in mcap cat
#1159
[go/cli] Fix uint64 bug in mcap cat
#1159
Conversation
Thanks for the fix! Could you add some tests for this? Looks like there are existing tests in https://github.com/foxglove/mcap/blob/main/go/cli/mcap/utils/ros/json_transcoder_test.go |
Kinda of interesting there already was a test, but... Running all the existing tests worked $ go test ./utils/ros -count=1
ok github.com/foxglove/mcap/go/cli/mcap/utils/ros 0.115s Running just the uint64 test didn't $ go test ./utils/ros -run TestSingleRecordConversion/uint64 -count=1
--- FAIL: TestSingleRecordConversion (0.00s)
--- FAIL: TestSingleRecordConversion/uint64 (0.00s)
json_transcoder_test.go:532:
Error Trace: /Users/bradsquicciarini/mcap/go/cli/mcap/utils/ros/json_transcoder_test.go:532
Error: Not equal:
expected: "{\"foo\":506381209866536711}"
actual : "{\"foo\":117901063}"
Diff:
--- Expected
+++ Actual
@@ -1 +1 @@
-{"foo":506381209866536711}
+{"foo":117901063}
Test: TestSingleRecordConversion/uint64
FAIL
FAIL github.com/foxglove/mcap/go/cli/mcap/utils/ros 0.117s
FAIL Looks like the test was reading some well placed garbage from prior tests. So just zeroed out the buffer between test cases. |
lol…amazing 🙌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix!
It looks like there's also an incorrect conversion a few lines below; would you like to deal with that in this PR (and maybe use a larger value in the test to catch it)?
https://github.com/foxglove/mcap/blob/main/go/cli/mcap/utils/ros/json_transcoder.go#L285
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you 👍
This ports the bugfix from foxglove/mcap#1159
@bryfox @jtbandes I ported this to the copy of this code at foxglove/go-rosbag#5 IIRC the foxglove CLI tool consumes that version so you will probably want to update its deps once that goes in. The MCAP CLI tool should probably also be updated to refer to that code instead. |
This ports the bugfix from foxglove/mcap#1159
Changelog
mcap cat
is not readinguint64
ros1 msgs properlyDescription
Super minor, but looks like the
JSONTranscoder
forros1msg
is only reading 4 bytes for theuint64
type.