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

test: primitives serde test with serde_json and bincode #1566

Merged
merged 4 commits into from
Jul 30, 2024
Merged

Conversation

dinhani-cw
Copy link
Contributor

@dinhani-cw dinhani-cw commented Jul 30, 2024

PR Type

Tests, Enhancement


Description

  • Added tests for BlockHeader methods including hash calculation and parent hash.
  • Introduced Hash::ZERO constant and removed the zero method.
  • Enhanced gen_test_serde macro to generate both serde_json and bincode tests.
  • Added serde tests for multiple primitives.
  • Relocated BlockHeader tests from mod.rs to block_header.rs.

Changes walkthrough 📝

Relevant files
Tests
block_header.rs
Add tests and modify `parent_hash` initialization in `BlockHeader`

src/eth/primitives/block_header.rs

  • Added tests for BlockHeader methods.
  • Modified parent_hash initialization to use Hash::ZERO.
  • +33/-1   
    mod.rs
    Add serde tests and relocate `BlockHeader` tests                 

    src/eth/primitives/mod.rs

  • Added serde tests for multiple primitives.
  • Removed and relocated BlockHeader tests.
  • +38/-29 
    Enhancement
    hash.rs
    Add `ZERO` constant and remove `zero` method in `Hash`     

    src/eth/primitives/hash.rs

  • Added constant ZERO to Hash.
  • Removed zero method from Hash.
  • +2/-5     
    ext.rs
    Enhance `gen_test_serde` macro to include bincode tests   

    src/ext.rs

  • Enhanced gen_test_serde macro to include bincode tests.
  • Added separate serde_json and bincode tests.
  • +20/-5   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @dinhani-cw dinhani-cw enabled auto-merge (squash) July 30, 2024 11:03
    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Unwrap Usage
    The code at line 68 uses unwrap_or(Hash::ZERO). It is recommended to use expect with a meaningful error message to avoid potential panics.

    Unwrap Usage
    The code at lines 310, 312, 322, and 324 uses unwrap. It is recommended to use expect with a meaningful error message to avoid potential panics.

    Copy link

    github-actions bot commented Jul 30, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Add error context to unwrap calls in the gen_test_serde macro to provide more informative error messages

    In the gen_test_serde macro, consider adding error context to unwrap calls to
    provide more informative error messages during test failures.

    src/ext.rs [310]

    -let value_encoded = serde_json::to_string(&value_original).unwrap();
    +let value_encoded = serde_json::to_string(&value_original).expect("Failed to serialize value to JSON");
     
    Suggestion importance[1-10]: 9

    Why: Adding error context to unwrap calls enhances maintainability by providing more informative error messages, which can be very helpful during debugging and test failures.

    9
    Performance
    Replace unwrap_or with unwrap_or_else to defer the creation of the default value

    The unwrap_or method can be replaced with unwrap_or_else to defer the creation of
    Hash::ZERO until it is needed, which can improve performance slightly.

    src/eth/primitives/block_header.rs [68]

    -parent_hash: number.prev().map(|n| n.hash()).unwrap_or(Hash::ZERO),
    +parent_hash: number.prev().map(|n| n.hash()).unwrap_or_else(|| Hash::ZERO),
     
    Suggestion importance[1-10]: 8

    Why: This change can improve performance slightly by deferring the creation of Hash::ZERO until it is needed. It is a minor but beneficial optimization.

    8
    Best practice
    Use a constant for the modulo divisor to avoid magic numbers and improve code clarity

    The into_hash_partition method can be optimized by using a constant for the modulo
    divisor to avoid magic numbers and improve code clarity.

    src/eth/primitives/hash.rs [36]

    -let n = self.0.to_low_u64_ne() % 10;
    +const PARTITION_DIVISOR: u64 = 10;
    +let n = self.0.to_low_u64_ne() % PARTITION_DIVISOR;
     
    Suggestion importance[1-10]: 7

    Why: Replacing magic numbers with named constants improves code clarity and maintainability by making the code more self-documenting.

    7
    Readability
    Use a more descriptive variable name in the test to improve readability

    In the test block_header_hash_calculation, consider using a more descriptive
    variable name instead of header to improve code readability.

    src/eth/primitives/block_header.rs [255]

    -let header = BlockHeader::new(BlockNumber::ZERO, UnixTime::from(1234567890));
    +let block_header = BlockHeader::new(BlockNumber::ZERO, UnixTime::from(1234567890));
     
    Suggestion importance[1-10]: 6

    Why: Using a more descriptive variable name improves code readability, which is beneficial for understanding and maintaining the code, especially in tests.

    6

    @dinhani-cw dinhani-cw disabled auto-merge July 30, 2024 11:06
    @dinhani-cw dinhani-cw enabled auto-merge (squash) July 30, 2024 11:10
    @dinhani-cw dinhani-cw disabled auto-merge July 30, 2024 11:19
    @dinhani-cw dinhani-cw enabled auto-merge (squash) July 30, 2024 11:21
    @dinhani-cw dinhani-cw merged commit a15303e into main Jul 30, 2024
    33 checks passed
    @dinhani-cw dinhani-cw deleted the test branch July 30, 2024 11:34
    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.

    1 participant