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: fix "cargo test" after cst change. #294

Merged
merged 1 commit into from
Jan 20, 2025

Conversation

wxsBSD
Copy link
Contributor

@wxsBSD wxsBSD commented Jan 18, 2025

I apparently don't understand enough about rust to know why it is running test code from comments, but it apparently is.

I apparently don't understand enough about rust to know why it is running test
code from comments, but it apparently is.
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Hi team,

I'm currently reviewing this pull request and will provide my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly understand the changes:

This pull request aims to fix the "cargo test" failures that arose after a change in the Concrete Syntax Tree (CST). The author, wxsBSD, mentions a lack of understanding of why test code was being run from comments, leading to this fix.

The changes involve modifying the parser/src/cst/mod.rs file. Specifically, the into_cst() method calls within the doc comments have been replaced with try_into_cst().unwrap(). This change appears in multiple locations within the doc comments of the Token and Node implementations:

  • Line 429, 468, 759, 803: into_cst() changed to try_into_cst().unwrap() in doc comments for Token methods.
  • Line 759, 803: into_cst() changed to try_into_cst().unwrap() in doc comments for Node methods.

This suggests a potential issue with error handling or the previous implementation's interaction with test code within comments. The try_into_cst() method likely provides more robust error handling, preventing the unexpected execution of test code.

I'll provide a more detailed review soon, but this summary should give you a head start.

And now, a little haiku to lighten the mood:

Tests fail, code's changed,
Comments run, a mystery solved,
Rust's secrets revealed.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

This change updates the example code in documentation comments to reflect the change from .into_cst() to .try_into_cst().unwrap(). It's good practice to keep documentation and code synchronized. There are no changes to the functionality of the code itself.

@@ -426,7 +426,8 @@ impl<M> Token<M> {
/// # use yara_x_parser::cst::SyntaxKind;
/// # use yara_x_parser::Parser;
/// let mut token = Parser::new(b"rule test {condition:true}")
/// .into_cst()
/// .try_into_cst()
Copy link

Choose a reason for hiding this comment

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

low

Since try_into_cst can return an error, it's good that you've added .unwrap() to handle the Result. However, the documentation example should ideally demonstrate proper error handling instead of simply unwrapping. Would you consider adding a check for the Result and handling the error gracefully in the example?

///     .try_into_cst()
///     .expect("failed to convert to CST")

@@ -465,7 +466,8 @@
/// # use yara_x_parser::cst::SyntaxKind;
/// # use yara_x_parser::Parser;
/// let mut token = Parser::new(b"rule test {condition:true}")
/// .into_cst()
/// .try_into_cst()
Copy link

Choose a reason for hiding this comment

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

low

Since try_into_cst can return an error, it's good that you've added .unwrap() to handle the Result. However, the documentation example should ideally demonstrate proper error handling instead of simply unwrapping. Would you consider adding a check for the Result and handling the error gracefully in the example?

///     .try_into_cst()
///     .expect("failed to convert to CST")

@@ -756,7 +758,8 @@
/// rule test_1 {condition:true}
/// rule test_2 {condition:true}
/// ")
/// .into_cst()
/// .try_into_cst()
Copy link

Choose a reason for hiding this comment

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

low

Since try_into_cst can return an error, it's good that you've added .unwrap() to handle the Result. However, the documentation example should ideally demonstrate proper error handling instead of simply unwrapping. Would you consider adding a check for the Result and handling the error gracefully in the example?

///     .try_into_cst()
///     .expect("failed to convert to CST")

@@ -800,7 +803,8 @@
/// rule test_1 {condition:true}
/// rule test_2 {condition:true}
/// ")
/// .into_cst()
/// .try_into_cst()
Copy link

Choose a reason for hiding this comment

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

low

Since try_into_cst can return an error, it's good that you've added .unwrap() to handle the Result. However, the documentation example should ideally demonstrate proper error handling instead of simply unwrapping. Would you consider adding a check for the Result and handling the error gracefully in the example?

///     .try_into_cst()
///     .expect("failed to convert to CST")

@plusvic
Copy link
Member

plusvic commented Jan 20, 2025

I apparently don't understand enough about rust to know why it is running test code from comments, but it apparently is.

Rust encourages you to write code examples that are also tests, precisely to prevent cases like this one, where the documentation gets out-of-date due to changes in the actual code.

@plusvic plusvic changed the title fix: Fix "cargo test" after cst change. fix: fix "cargo test" after cst change. Jan 20, 2025
@plusvic plusvic changed the title fix: fix "cargo test" after cst change. tests: fix "cargo test" after cst change. Jan 20, 2025
@plusvic plusvic changed the title tests: fix "cargo test" after cst change. test: fix "cargo test" after cst change. Jan 20, 2025
@plusvic plusvic merged commit 8a517a6 into VirusTotal:main Jan 20, 2025
17 of 18 checks passed
@wxsBSD wxsBSD deleted the fix_tests_after_cst_change branch January 20, 2025 11:58
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