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

added unit tests for data transformer types #2876

Merged
merged 26 commits into from
Feb 5, 2025

Conversation

Nityam573
Copy link
Contributor

@Nityam573 Nityam573 commented Jan 25, 2025

Closes #2575

Introduced changes

Added u384 tests, u96 tests and bytes31 tests

Checklist

  • ✅ Linked relevant issue
  • ✅ Updated relevant documentation
  • ✅ Added relevant tests
  • ✅ Performed self-review of the code
  • ✅ Added changes to CHANGELOG.md

@franciszekjob
Copy link
Collaborator

Hi @Nityam573 👋,
let me if you're ready for review or need any assistance 😄

@franciszekjob
Copy link
Collaborator

franciszekjob commented Jan 28, 2025

Hi @Nityam573 😄,
I see you've added test for u384, are you going to add tests for other types?

Also, please fix formatting and lint, by running cargo fmt and cargo lint as described in the development docs

@Nityam573
Copy link
Contributor Author

@franciszekjob i am on it will ready for review by tonight

@Nityam573 Nityam573 marked this pull request as ready for review January 28, 2025 17:34
@Nityam573
Copy link
Contributor Author

@franciszekjob please review

@Nityam573 Nityam573 changed the title added u384 test added unit tests for data transformer types Jan 28, 2025
crates/data-transformer/tests/u96.rs Outdated Show resolved Hide resolved
crates/data-transformer/tests/u384.rs Show resolved Hide resolved
crates/data-transformer/tests/u96.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
crates/data-transformer/tests/bytes31.rs Outdated Show resolved Hide resolved
@Nityam573
Copy link
Contributor Author

@franciszekjob I am getting some issues while testing u384.rs can i configure u384.rs file from my end and then test it

@franciszekjob
Copy link
Collaborator

@Nityam573 What do you mean by "configure u384.rs file from my end"? 😅

@Nityam573
Copy link
Contributor Author

Nityam573 commented Jan 31, 2025

@franciszekjob The error is happening when trying to convert slices to arrays in the from_bytes function in u384
I guess error is in the from_bytes implementation trying to convert 12 bytes into u128 (which needs 16 bytes)
What is your say regarding it

@franciszekjob
Copy link
Collaborator

franciszekjob commented Feb 1, 2025

@franciszekjob The error is happening when trying to convert slices to arrays in the from_bytes function in u384 I guess error is in the from_bytes implementation trying to convert 12 bytes into u128 (which needs 16 bytes) What is your say regarding it

@Nityam573 yes, you're correct 👍
Could you please modify CairoU384.from_bytes implementation? It should look as follows:

impl CairoU384 {
    #[must_use]
    pub fn from_bytes(bytes: &[u8; 48]) -> Self {
        fn to_u128(slice: &[u8]) -> u128 {
            let mut padded = [0u8; 16];
            padded[4..].copy_from_slice(slice);
            u128::from_be_bytes(padded)
        }

        Self {
            limb_0: to_u128(&bytes[36..48]),
            limb_1: to_u128(&bytes[24..36]),
            limb_2: to_u128(&bytes[12..24]),
            limb_3: to_u128(&bytes[0..12]),
        }
    }
}

I've added extra 4 bytes in size for each slice, should be fine now.

@Nityam573
Copy link
Contributor Author

@franciszekjob please review

crates/data-transformer/tests/bytes31.rs Outdated Show resolved Hide resolved
crates/data-transformer/tests/bytes31.rs Outdated Show resolved Hide resolved
crates/data-transformer/tests/u384.rs Outdated Show resolved Hide resolved
crates/data-transformer/tests/u96.rs Outdated Show resolved Hide resolved
crates/data-transformer/tests/u96.rs Outdated Show resolved Hide resolved
@Nityam573
Copy link
Contributor Author

@franciszekjob please review

crates/data-transformer/tests/bytes31.rs Outdated Show resolved Hide resolved
crates/data-transformer/tests/u384.rs Outdated Show resolved Hide resolved
crates/data-transformer/tests/u384.rs Outdated Show resolved Hide resolved
crates/data-transformer/tests/u384.rs Outdated Show resolved Hide resolved
crates/data-transformer/tests/u96.rs Outdated Show resolved Hide resolved
crates/data-transformer/tests/u96.rs Outdated Show resolved Hide resolved
@Nityam573
Copy link
Contributor Author

@franciszekjob please review

@Nityam573
Copy link
Contributor Author

@franciszekjob review the changes

@Nityam573
Copy link
Contributor Author

@franciszekjob review please

Copy link
Collaborator

@franciszekjob franciszekjob left a comment

Choose a reason for hiding this comment

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

Otherwise looks fine, @cptartur @ddoktorski could one of you also take a look?

crates/data-transformer/tests/bytes31.rs Outdated Show resolved Hide resolved
@Nityam573
Copy link
Contributor Author

Nityam573 commented Feb 4, 2025

@cptartur @ddoktorski @kkawula please review

@Nityam573
Copy link
Contributor Author

Nityam573 commented Feb 4, 2025

@cptartur @kkawula PLEASE review and merge the PR

@cptartur cptartur enabled auto-merge February 5, 2025 17:06
@cptartur cptartur added this pull request to the merge queue Feb 5, 2025
Merged via the queue into foundry-rs:master with commit 84e5908 Feb 5, 2025
38 checks passed
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.

Add unit tests for data transformer types
3 participants