Skip to content

Conversation

@Weixing-Zhang
Copy link
Collaborator

Implemented:

  • Added a unit test to verify that rust and cpp compression results match
  • Added a test data generator to cover diverse input patterns for better test coverage

@Weixing-Zhang Weixing-Zhang force-pushed the test/rust-and-cpp-compression-matches branch from 8ff21b3 to dc87145 Compare May 6, 2025 05:28
@codecov
Copy link

codecov bot commented May 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@@ -0,0 +1,45 @@
#![cfg(all(feature = "rust", feature = "cpp"))]
Copy link
Member

Choose a reason for hiding this comment

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

i think its better to move this to tests/common.rs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

);
assert_eq!(decoded_cpp, decoded_by_rust);

for input in get_test_cases(512 + rust::BLOCK_SIZE_128 as usize) {
Copy link
Member

Choose a reason for hiding this comment

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

while totally not critical, and probably good for another PR, but it seems it might benefit us in the future to come up with some trait-based API that is the same for both rust and cpp implementation... ideas are welcome

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Completely agreed. For now, I need to apply a small workaround due to the interface between the rust-native implementation and the rust wrapper. A key issue is that the current rust-native code does not support arbitrary input lengths. The cpp implementation appears to tolerate such cases. I have added this to my to-do list once I better understand the algorithm.

Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

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

thanks!

@nyurik nyurik merged commit c776812 into fast-pack:main May 7, 2025
6 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.

2 participants