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

Clippy fixes v2 #10124

Closed
wants to merge 6 commits into from
Closed

Clippy fixes v2 #10124

wants to merge 6 commits into from

Conversation

catenacyber
Copy link
Contributor

Link to redmine ticket:
None

Describe changes:

  • rust: fix clippy warnings in tests

@jasonish why do I get these and CI is not red ?
Is this because the lint level to deny warnings does not apply to test code ?

#10113 rebased and more commits
Only 19 match_single_binding warnings remaining

Which will be optimized away by the compiler
using panic! instead with a string message
Coming from old test_case crate

warning: unneeded unit expression
   --> src/bittorrent_dht/parser.rs:590:5
    |
590 | /     #[test_case(
591 | |         b"",
592 | |         "Error: discovered Dict but expected EOF" ;
593 | |         "test parse bittorrent dht packet err 1"
594 | |     )]
    | |______^
warning: this is a decimal constant
   --> src/mqtt/parser.rs:888:19
    |
888 |             0x00, 06, /* Topic Length: 6 */
    |                   ^^
    |
warning: calls to `push` immediately after creation
    --> src/pgsql/parser.rs:1179:9
     |
1179 | /         let mut database_param: Vec<PgsqlParameter> = Vec::new();
1180 | |         database_param.push(database);
     | |______________________________________^
help: consider using the `vec![]` macro: `let database_param: Vec<PgsqlParameter> = vec![..];`
warning: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let`
   --> src/http2/parser.rs:882:17
    |
882 | /                 match ctx.value {
883 | |                     Some(_) => {
884 | |                         panic!("Unexpected value");
885 | |                     }
886 | |                     None => {}
887 | |                 }
    | |_________________^
Copy link

codecov bot commented Jan 5, 2024

Codecov Report

Attention: 44 lines in your changes are missing coverage. Please review.

Comparison is base (a37fa62) 82.15% compared to head (f9088a6) 82.16%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10124      +/-   ##
==========================================
+ Coverage   82.15%   82.16%   +0.01%     
==========================================
  Files         974      974              
  Lines      271925   271856      -69     
==========================================
- Hits       223394   223373      -21     
+ Misses      48531    48483      -48     
Flag Coverage Δ
fuzzcorpus 62.96% <ø> (+0.06%) ⬆️
suricata-verify 61.41% <ø> (-0.04%) ⬇️
unittests 62.83% <77.08%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Comment on lines +228 to 232
if let Some(txp) = parse_tftp_request(&READ_REQUEST[..]) {
assert_eq!(tx, txp);
} else {
panic!("Result should not be None");
}
Copy link
Member

Choose a reason for hiding this comment

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

In tests I prefer:

        let txp = parse_tftp_request(&READ_REQUEST[..]).unwrap();
        assert_eq!(tx, txp);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice

Comment on lines +641 to 645
if let Ok((_, val)) = parse_bytemath("bytes 4, offset 3933, oper +, rvalue myrvalue, result foo, string hex") {
assert_eq!(val, bmd);
} else {
panic!("Result shoud be ok");
}
Copy link
Member

Choose a reason for hiding this comment

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

Again, could just unwrap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed

Comment on lines +413 to 415
if parser::parse_dcerpc_udp_header(request).is_ok() {
panic!("Result shoud not be ok");
}
Copy link
Member

Choose a reason for hiding this comment

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

assert!(parser::parser_dcerpc_udp_header(request).is_err());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link
Member

@jasonish jasonish left a comment

Choose a reason for hiding this comment

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

Happy to see this ints being fixed up, but I think we should opt for asserts and even unwraps/expects before conditional panics.

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 17375

@catenacyber catenacyber mentioned this pull request Jan 6, 2024
@catenacyber
Copy link
Contributor Author

Replaces by #10129, thanks Jason for the review

@catenacyber catenacyber closed this Jan 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants