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

Migration package tests #8524

Merged
merged 12 commits into from
Mar 1, 2021
Merged

Migration package tests #8524

merged 12 commits into from
Mar 1, 2021

Conversation

rkapka
Copy link
Contributor

@rkapka rkapka commented Feb 26, 2021

What type of PR is this?

Testing

What does this PR do? Why is it needed?

The migration package, used for mapping between new and old proto objects, did not contain any tests. The PR addresses this repugnant state of affairs.

Which issues(s) does this PR fix?

N/A

Other notes for review

N/A

@rkapka rkapka requested a review from a team as a code owner February 26, 2021 07:18
@rkapka rkapka added API Api related tasks Ready For Review A pull request ready for code review OK to merge labels Feb 26, 2021
Copy link
Member

@nisdas nisdas left a comment

Choose a reason for hiding this comment

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

One canonical way to verify that you properly copied the object would be to retrieve its root. So you could just verify that the root is the same for both methods.


// HydrateV1SignedBeaconBlock hydrates a signed beacon block with correct field length sizes
// to comply with fssz marshalling and unmarshalling rules.
func HydrateV1SignedBeaconBlock(b *v1.SignedBeaconBlock) *v1.SignedBeaconBlock {
Copy link
Member

Choose a reason for hiding this comment

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

Test for this? : )

TestHydrateSignedBeaconBlock_NoError for reference

@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

❗ No coverage uploaded for pull request base (develop@c30ee6c). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             develop    #8524   +/-   ##
==========================================
  Coverage           ?   58.16%           
==========================================
  Files              ?      460           
  Lines              ?    33044           
  Branches           ?        0           
==========================================
  Hits               ?    19219           
  Misses             ?    10905           
  Partials           ?     2920           

@prylabs-bulldozer prylabs-bulldozer bot merged commit 9547f53 into develop Mar 1, 2021
@delete-merged-branch delete-merged-branch bot deleted the migration-tests branch March 1, 2021 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Api related tasks Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants