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

Task 4053, convert unit tests to FAIL/PASS API #5497

Closed

Conversation

jufajardini
Copy link
Contributor

detect/msg: convert to FAIL/PASS API

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

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

Describe changes:

  • detect/msg: convert to FAIL/PASS API
  • Partially closes redmine ticket 2975

PRScript output (if applicable):

#suricata-verify-pr:
#suricata-verify-repo:
#suricata-verify-branch:
#suricata-update-pr:
#suricata-update-repo:
#suricata-update-branch:
#libhtp-pr:
#libhtp-repo:
#libhtp-branch:

detect/msg: convert to FAIL/PASS API
@jufajardini jufajardini requested a review from a team as a code owner October 18, 2020 12:23
@victorjulien
Copy link
Member

In the git commit message, "detect/msg: convert to FAIL/PASS API" should be the subject line. Referencing to a ticket would be something like "Issue #xxxx" in the message body

Signature *sig = NULL;
const char *teststringparsed = "flow stateless to_server";
DetectEngineCtx *de_ctx = DetectEngineCtxInit();
if (de_ctx == NULL)
goto end;
FAIL_IF(de_ctx == NULL);
Copy link
Member

Choose a reason for hiding this comment

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

We have a slightly cleaner macro for checking pointers: FAIL_IF_NULL(ptr).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood! I saw that option at first, but wasn't really sure it was ready for using, because of the issue open that seems to be related to that...

Signature *sig = NULL;
const char *teststringparsed = "flow stateless to_server";
DetectEngineCtx *de_ctx = DetectEngineCtxInit();
if (de_ctx == NULL)
goto end;
FAIL_IF(de_ctx == NULL);

FILE *fd = SCClassConfGenerateValidDummyClassConfigFD01();
SCClassConfLoadClassficationConfigFile(de_ctx, fd);

sig = SigInit(de_ctx, "alert tcp any any -> any any (msg:\"flow stateless to_server\"; flow:stateless,to_server; content:\"flowstatelesscheck\"; classtype:bad-unknown; sid: 40000002; rev: 1;)");
Copy link
Member

Choose a reason for hiding this comment

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

extra points for: declare sig here. This leads to more compact tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, thanks. Following this logic, could lines

FILE *fd = SCClassConfGenerateValidDummyClassConfigFD01();
SCClassConfLoadClassficationConfigFile(de_ctx, fd);

Be moved to the end, where there used to be the check

if(sig != NULL)

I don't see fd being directly used anywhere else, which makes me believe they could. I have checked SCClassConfLoadClassficationConfigFile and it seems it will only use de_ctx, not change it. So it seems that this is something that could sort of be together with the other statements near

PASS

Does that make sense?

@jufajardini
Copy link
Contributor Author

Thanks for the overall comments. I believe I have done the appropriate changes, and I'll make a new PR now.

@jufajardini jufajardini deleted the fail-pass-api-task-4053-v2 branch December 11, 2020 20:47
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.

2 participants