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

detect/secresult: convert unittest to FAIL/PASS APIs #6563

Conversation

TheKharleeci
Copy link
Contributor

@TheKharleeci TheKharleeci commented Nov 2, 2021

Make sure these boxes are signed before submitting your Pull Request -- thank you.

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/4055

Ticket: #4055

Previous PR: #6562

Describe changes:

  • Update detect-rfb-secresult unit tests to use FAIL/PASS APIs
  • Update Copyright Year

@TheKharleeci TheKharleeci requested a review from a team as a code owner November 2, 2021 14:57
@codecov
Copy link

codecov bot commented Nov 2, 2021

Codecov Report

Merging #6563 (757b2bf) into master (a87c7e5) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6563      +/-   ##
==========================================
- Coverage   77.04%   77.02%   -0.02%     
==========================================
  Files         613      613              
  Lines      186693   186686       -7     
==========================================
- Hits       143833   143793      -40     
- Misses      42860    42893      +33     
Flag Coverage Δ
fuzzcorpus 52.90% <ø> (-0.02%) ⬇️
suricata-verify 52.07% <ø> (-0.03%) ⬇️
unittests 63.11% <100.00%> (+<0.01%) ⬆️

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

Copy link
Contributor

@jufajardini jufajardini left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, a few nit comments.

I'm not sure if it is ok or not to suppress the rfb here in the commit message, so that might need change...

@@ -1,4 +1,4 @@
/* Copyright (C) 2020 Open Information Security Foundation
/* Copyright (C) 2021 Open Information Security Foundation
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for taking the time to also update the copyright year :)

One thing is that they must be updated in such a way that it covers their existence, if I can put it that way. That means that here we should have 2020-2021, not erasing 2020.

@@ -268,14 +268,13 @@ void DetectRfbSecresultFree(DetectEngineCtx *de_ctx, void *de_ptr)
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

It is preferred to remove the retvals, now.
(so, deleting lines 266 and 267)

Copy link
Contributor

Choose a reason for hiding this comment

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

Same applies to the other test. :)


FAIL_IF_NOT_NULL(de);

DetectRfbSecresultFree(NULL, de);
Copy link
Contributor

Choose a reason for hiding this comment

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

One review I received just the other day and which I believe applies here is that we can remove this DetectRfbSecresultFree call. The reasoning behind it is that if it is not null, we won't reach this call, and if we reach it... then it should be null, already...

@jasonish
Copy link
Member

jasonish commented Nov 2, 2021

I'm not sure if it is ok or not to suppress the rfb here in the commit message, so that might need change...

It would be better if it was still in there: detect-rfb-secresult: ...

@TheKharleeci
Copy link
Contributor Author

Followed by: #6565

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