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

feat: hesai timezone unit tests #81

Merged
merged 6 commits into from
Oct 17, 2023

Conversation

mojomex
Copy link
Collaborator

@mojomex mojomex commented Oct 2, 2023

PR Type

  • Improvement

Related Links

#78 fix: hesai_decoder replace mktime with timegm -- the motivation for this PR
#80 Generic hesai unit tests -- unit test refactoring which this PR depends on

Description

Depends on #80 Generic hesai unit tests.

This PR implements a unit test for all Hesai sensor models which tests for correct timezone handling.
The expected decoder behavior is that, regardless of system timezone settings, pointcloud timestamps
are output in UST.
As demonstrated by #78, using the wrong function to parse timestamps can lead to timezone-dependent behavior,
which was, until now, hard to catch during development.

The new unit test sets the process' timezone settings to different values and decodes the same rosbag in each of them.
The pointcloud timestamps as returned by HesaiDriver::ConvertScanToPointcloud, decoded in different timezones, are then checked for equality.

Review Procedure

Change between mktime and gmtime in line 59 in hesai_packet.hpp, compile and run the unit tests.
They should fail for mktime and pass for timegm, as pointed out in #78.

With timegm:

1: [       OK ]

With mktime:

1: .../hesai_ros_decoder_test_main.cpp:137: Failure
1: Expected equality of these values:
1:   decoded_timestamps.back()
1:     Which is: 1504682387
1:   decoded_timestamps_cmp.back()
1:     Which is: 1504714787
...
1: [  FAILED  ]

The timestamps can be converted to human-readable form in the terminal using date -ud @<timestamp>:

$ date -ud @1504682387
2017年  9月  6日 水曜日 07:19:47 UTC
$ date -ud @1504714787
2017年  9月  6日 水曜日 16:19:47 UTC

Remarks

Pre-Review Checklist for the PR Author

PR Author should check the checkboxes below when creating the PR.

  • Assign PR to reviewer

Checklist for the PR Reviewer

Reviewers should check the checkboxes below before approval.

  • Commits are properly organized and messages are according to the guideline
  • (Optional) Unit tests have been written for new behavior
  • PR title describes the changes

Post-Review Checklist for the PR Author

PR Author should check the checkboxes below before merging.

  • All open points are addressed and tracked via issues or tickets

CI Checks

  • Build and test for PR: Required to pass before the merge.

The sensor-specific duplicates of the ..._test_main and ..._test files
have been refactored into a single generic implementation that takes
only the sensor parameters as input.

The ReadBag function has been refactored such that it takes a callback
function which is called on every point cloud scan.
This allows for easy and concise implementation of new test cases.
Decoders should always decode pointcloud timestamps in UST.
This new unit test decodes the same pointcloud in different timezones
and checks if the decoded times are equal.
@mojomex mojomex requested review from amc-nu and drwnz October 2, 2023 06:22
@mojomex mojomex self-assigned this Oct 2, 2023
@codecov-commenter
Copy link

codecov-commenter commented Oct 2, 2023

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (6ceea8d) 8.03% compared to head (e6ac003) 31.80%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##            main      #81       +/-   ##
==========================================
+ Coverage   8.03%   31.80%   +23.76%     
==========================================
  Files        108        7      -101     
  Lines       9069      742     -8327     
  Branches     844      463      -381     
==========================================
- Hits         729      236      -493     
+ Misses      7778       94     -7684     
+ Partials     562      412      -150     
Flag Coverage Δ
differential 31.80% <25.00%> (?)
total ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
nebula_tests/hesai/hesai_ros_decoder_test_main.cpp 31.25% <25.00%> (-2.85%) ⬇️

... and 101 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@amc-nu
Copy link
Contributor

amc-nu commented Oct 17, 2023

@mojomex We tested this branch, and it works correctly. Thank you.
Could you please resolve the conflicts raised with #80

@mojomex
Copy link
Collaborator Author

mojomex commented Oct 17, 2023

@amc-nu All resolved, thank you for reviewing!

@amc-nu amc-nu merged commit 4010d45 into tier4:main Oct 17, 2023
6 checks passed
@mojomex mojomex deleted the hesai-timezone-unit-tests branch October 17, 2023 06:56
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.

3 participants