Skip to content

Conversation

Ifechukwudaniel
Copy link
Contributor

@Ifechukwudaniel Ifechukwudaniel commented Jul 9, 2025

This PR standardizes test function names across the codebase to follow a consistent naming pattern:

  • function_name_description for successful cases
  • function_name_revert_when_condition for expected reverts

This improves clarity, consistency and makes it easier to understand the purpose of each test at a glance.

Closes: 491


Checklist:

  • Tests updated or renamed as needed
  • Documentation (N/A)
  • Changelog updated

@Ifechukwudaniel Ifechukwudaniel marked this pull request as draft July 9, 2025 15:24
Copy link

netlify bot commented Jul 9, 2025

Deploy Preview for contracts-stylus canceled.

Name Link
🔨 Latest commit 597708b
🔍 Latest deploy log https://app.netlify.com/projects/contracts-stylus/deploys/68abac233aedc600083054f4

@Ifechukwudaniel Ifechukwudaniel marked this pull request as ready for review July 11, 2025 13:52
@Ifechukwudaniel
Copy link
Contributor Author

Ifechukwudaniel commented Jul 11, 2025

@0xNeshi , @bidzyyys , @qalisander

@Ifechukwudaniel Ifechukwudaniel marked this pull request as draft July 11, 2025 15:42
@Ifechukwudaniel Ifechukwudaniel marked this pull request as draft July 11, 2025 15:42
@Ifechukwudaniel Ifechukwudaniel marked this pull request as draft July 11, 2025 15:42
@Ifechukwudaniel Ifechukwudaniel marked this pull request as ready for review July 12, 2025 14:39
@0xNeshi 0xNeshi mentioned this pull request Jul 14, 2025
1 task
@Ifechukwudaniel
Copy link
Contributor Author

@0xNeshi, can you please review this so I can do the next one

@0xNeshi
Copy link
Collaborator

0xNeshi commented Jul 15, 2025

Hey @Ifechukwudaniel , appreciate the enthusiasm! We're currently under a huge workload, and will review the PR as soon as it is reduced. Thanks!

Copy link
Collaborator

@0xNeshi 0xNeshi left a comment

Choose a reason for hiding this comment

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

Good job. Left some comments.

Copy link
Collaborator

@0xNeshi 0xNeshi left a comment

Choose a reason for hiding this comment

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

Please resolve merge conflicts

@@ -544,7 +548,7 @@ async fn error_invalid_array_length_in_batch_mint(
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change errors_when_invalid_receiver_contract_in_batch_mint to mint_batch_reverts_when_receiver_invalid

@@ -544,7 +548,7 @@ async fn error_invalid_array_length_in_batch_mint(
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Change error_invalid_array_length_in_batch_mint to mint_batch_reverts_when_array_length_invalid

Copy link
Member

@qalisander qalisander left a comment

Choose a reason for hiding this comment

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

Good work!

I see the second part of convention function_name_revert_when_condition is not applicable for crypto library. Namely revert_when syntax relates to smart contracts domain only.

@@ -1052,7 +1052,7 @@ mod tests {
const MODULUS: i128 = 1000003; // Prime number

#[test]
fn add() {
fn add_succeeds_for_field_elements() {
Copy link
Member

@qalisander qalisander Aug 9, 2025

Choose a reason for hiding this comment

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

These two test cases have an inconsistent description: "succeeds_for_field_elements" and "succeeds_with_expected_value" but test cases are quite similar.

@0xNeshi is "description" mandatory in our convention for successful tests? (function_name_description).
Can't we leave it as "" here??xD

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm ok with leaving just add for simple test cases OR ones that test the same function from multiple vectors

@Ifechukwudaniel
Copy link
Contributor Author

On it, i will complete it

Ifechukwudaniel and others added 22 commits August 20, 2025 19:31
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.

4 participants