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

Bug/exp overflow master #419

Merged
merged 2 commits into from
Dec 2, 2019
Merged

Bug/exp overflow master #419

merged 2 commits into from
Dec 2, 2019

Conversation

mike-burrage-hedera
Copy link
Contributor

Detailed description:

Applying bug fix from #409 to master.

From the original PR:

  • In Entities.create/update, handle overflows during conversion of (seconds,nanos) to a long nanos timestamp and set the database values accordingly.
  • Utilities.convertToNanos changed due to its implementation adding an additional runtime exception possibility (DateTimeException), rather than just the expected ArithmeticException.
  • Tested update via RecordFileLoggerCryptoTest and RecordFileLoggerFileTest

Additions in this change that were not on the release/0.4 branch:

  • Correcting a comment in RecordFileLoggerCryptoTest (on numeric overflow of expiration_ns, long max and min values are used, not null which indicates the value is unknown).
  • Readding rest-api test data that includes a null expiration timestamp result as well as still testing the new max and min long values that represent the overflow case.
  • Refactoring the rest integration test data setup which added confusion during the bug fix. json file integration_test_data.json along with comments at the top of integration.test.js should hopefully make this a bit more clear.

Which issue(s) this PR fixes:
Fixes #386

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated

@mike-burrage-hedera mike-burrage-hedera added bug Type: Something isn't working P1 labels Dec 2, 2019
@mike-burrage-hedera mike-burrage-hedera added this to the Mirror 0.5.0 milestone Dec 2, 2019
mike-burrage-hedera and others added 2 commits December 2, 2019 12:21
Signed-off-by: Mike Burrage <mike.burrage@hedera.com>
Ensure expiration_ns null value test is still in the rest integration test.
Make the integration test data setup and spec more clear/explicit by using a __tests__/integration_test_data.json data setup file, and adding more comments.

Signed-off-by: Mike Burrage <mike.burrage@hedera.com>
@codecov
Copy link

codecov bot commented Dec 2, 2019

Codecov Report

Merging #419 into master will increase coverage by 0.09%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #419      +/-   ##
==========================================
+ Coverage   57.65%   57.74%   +0.09%     
==========================================
  Files          56       56              
  Lines        2477     2480       +3     
  Branches      323      324       +1     
==========================================
+ Hits         1428     1432       +4     
- Misses        902      903       +1     
+ Partials      147      145       -2
Impacted Files Coverage Δ
...hedera/mirror/importer/parser/record/Entities.java 85.25% <100%> (ø) ⬆️
.../java/com/hedera/mirror/importer/util/Utility.java 56.81% <83.33%> (+0.09%) ⬆️
...irror/importer/parser/record/RecordFileLogger.java 84.19% <0%> (+0.45%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7abd912...ee078b6. Read the comment docs.

Copy link
Member

@steven-sheehy steven-sheehy left a comment

Choose a reason for hiding this comment

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

LGTM. As discussed, I think we'll need input files to be specifiable per spec and more input tables to be available, but that can wait for another PR.

@steven-sheehy steven-sheehy added the parser Area: File parsing label Dec 2, 2019
@steven-sheehy steven-sheehy merged commit 0e6972a into master Dec 2, 2019
@steven-sheehy steven-sheehy deleted the bug/exp-overflow-master branch December 2, 2019 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Type: Something isn't working P1 parser Area: File parsing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Long overflow when future expiration time
2 participants