Skip to content

Conversation

@Cherrypick14
Copy link

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

Contribution style:

Our Contribution agreements:

Changes (if applicable):

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

Describe changes:

  • Updated DetectPcreTestSig01 unittest to use the new FAIL_IF/PASS
  • API instead of the old return 0/1 pattern.
  • Added FAIL_IF_NULL check
  • for packet creation and simplified cleanup logic.

Provide values to any of the below to override the defaults.

  • To use a Suricata-Verify or Suricata-Update pull request,
    link to the pull request in the respective _BRANCH variable.
  • Leave unused overrides blank or remove.

SV_REPO=
SV_BRANCH=
SU_REPO=
SU_BRANCH=

@github-actions
Copy link

NOTE: This PR may contain new authors.

Copy link
Member

@victorjulien victorjulien left a comment

Choose a reason for hiding this comment

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

Formatting check fails as there is a space too many on 1588.

I also think that a PR for just a single test while there are more tests to do in the same file is a bit too much.

@victorjulien victorjulien added the outreachy Contributions made by Outreachy applicants label Oct 29, 2025
@codecov
Copy link

codecov bot commented Oct 29, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.19%. Comparing base (16d124c) to head (b3680b8).
⚠️ Report is 196 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14184      +/-   ##
==========================================
+ Coverage   83.87%   84.19%   +0.32%     
==========================================
  Files        1011     1013       +2     
  Lines      275671   262226   -13445     
==========================================
- Hits       231207   220770   -10437     
+ Misses      44464    41456    -3008     
Flag Coverage Δ
fuzzcorpus 63.39% <ø> (-0.12%) ⬇️
livemode 18.73% <ø> (-0.73%) ⬇️
pcap 44.57% <ø> (-0.20%) ⬇️
suricata-verify 64.84% <ø> (-0.31%) ⬇️
unittests 59.20% <100.00%> (+0.06%) ⬆️

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

To complement Victor's review:

Please ensure that all tests are now fully following the FAIL/ PASS API approach.
If you check bdf292b for instance you'll have some examples of what can be improved here.

Note, too, that once all possible FAIL possibilities have been checked, the tests should call any Freeing loginc and PASS. We don't want if branches in the unittests, once this conversion is done.

And please remember to add the ticket number to the commit message ;)

@Cherrypick14
Copy link
Author

Cherrypick14 commented Oct 29, 2025

Thank you @victorjulien and @jufajardini for the feedback. I just wanted to confirm one last thing. So for the new PR with feedback that I'll open that means I'll have to convert it to a draft right? until all tests have fully followed the FAIL/ PASS API approach for this ticket.

@samfajobi
Copy link
Contributor

samfajobi commented Oct 30, 2025

Thank you @victorjulien and @jufajardini for the feedback. I just wanted to confirm one last thing. So for the new PR with feedback that I'll open that means I'll have to convert it to a draft right? until all tests have fully followed the FAIL/ PASS API approach for this ticket.

Hi Cherry, I am an intern just like you, just trying to help out in a little way I can, you can submit as a normal PR, just ensure that you incorporate all feedbacks from mentors into your new PR, the clang-format failed, so after you have made changes to your branch, you need to use the clang-format to format your branch use ./scripts/clang-format.sh check-branch this helps to catch and get rid of unnecessary spaces you might have used accidentally, if it catches any then use the ./scripts/clang-format branch.sh to format.

As as regards your commit message, try checking out the doc on commit message, there has to be an empty space between your commit title and commit message body, for instance:

detect-pcre: convert unittests to FAIL/PASS APIs

Ticket: #6433

I hope this helps

@jufajardini
Copy link
Contributor

Followed by: #14220

@jufajardini jufajardini closed this Nov 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

outreachy Contributions made by Outreachy applicants

Development

Successfully merging this pull request may close these issues.

4 participants