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

Add test for PeerIdTooLong and clean up code #541

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

HasanZaigam
Copy link

PR Title:

Add Test for PeerIdTooLong Error and Clean Up Code in Network Authorization Pallet


Description:

This PR enhances the pallet-network-authorization module by adding tests for the PeerIdTooLong error scenario and refactoring the code to improve readability and maintainability.

Approach:

  1. Test for PeerIdTooLong Error: A test case is added to ensure that when a peer ID exceeds 64 bytes, it triggers the PeerIdTooLong error, validating the correct error handling.
  2. Successful Authorization Test: Another test ensures that a valid peer ID (64 bytes) is correctly authorized, confirming the function works as expected for valid inputs.
  3. Code Cleanup: Removed commented-out code to improve clarity and maintainability. The mock environment setup was also simplified.

Why This Matters:

  • Better Test Coverage: This PR ensures edge cases are covered, specifically for error handling related to peer ID validation.
  • Code Quality: Cleaning up commented code improves readability, making the codebase more maintainable.
  • Consistency: These changes ensure the pallet behaves consistently when handling valid and invalid peer IDs.

How to Test:

To run the tests, use the following command:

cargo test -p pallet-network-authorization -- --nocapture

This will verify that the error handling and successful authorization logic are working correctly.


By merging this PR, the code becomes more robust and easier to maintain, while laying the foundation for additional tests in the future.

@vatsa287
Copy link
Member

@HasanZaigam
The test PeerIdTooLong belongs to node-authorisation module.
There is already a stale PR on this, refer it and re-comit it.
#499

@HasanZaigam
Copy link
Author

Hi @vatsa287, I am confused now about what I have to do. Please tell me, if possible, if you can take a Google Meet. I didn't understand what I had to do next.

Copy link

@Hasan2k5 Hasan2k5 left a comment

Choose a reason for hiding this comment

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

Looks good overall! The function is clear, and the length check for peer_id is well-implemented. Just noticed some minor indentation issues—fixing that would make it more readable. Nice work!

@vatsa287
Copy link
Member

@HasanZaigam @Hasan2k5
You can ping me on discord vatsa_dhiway, for more clarifications.

Cargo.lock Show resolved Hide resolved
Cargo.toml Outdated
@@ -20,6 +20,7 @@ members = [
"pallets/did",
Copy link
Member

Choose a reason for hiding this comment

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

Changes made here is not required as well.

@@ -178,7 +178,7 @@ where
interval.tick().await;

if full_nodes.iter().all(|(id, service, _, _)| full_predicate(*id, service)) {
break
break;
Copy link
Member

Choose a reason for hiding this comment

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

This change is not required as it is the last statement in the block in Rust.

pallets/node-authorization/src/tests.rs Show resolved Hide resolved
Copy link
Author

@HasanZaigam HasanZaigam left a comment

Choose a reason for hiding this comment

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

I have made all the changes and improved the test.rs code to make the test for PeerIdTooLong more comprehensive, as per your guidance. Please review it again, sir.

Removed the network-authorization Module,so  that the code remove smoothly.
@HasanZaigam
Copy link
Author

@vatsa287, please check the changes and let me know , what next changes I have to make in it, please guide me .

@vatsa287
Copy link
Member

@HasanZaigam
All other changes other than tests.rs is not required.

@HasanZaigam HasanZaigam requested a review from vatsa287 January 3, 2025 18:19
@vatsa287
Copy link
Member

vatsa287 commented Jan 5, 2025

@HasanZaigam All other changes other than tests.rs is not required.

@HasanZaigam ^^

@HasanZaigam
Copy link
Author

ok , i makes all changes write and add the test case in test only and let you know, after making the changes.

@HasanZaigam
Copy link
Author

@vatsa287 makes the changes please review it

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.

3 participants