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.

Sign up for GitHub

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

Jump to bottom

test/stream: Update drop reason per new reason code #1357

Closed
wants to merge 1 commit into from
Closed

test/stream: Update drop reason per new reason code #1357

wants to merge 1 commit into from

Conversation

jlucovsky
Copy link
Contributor

Issue: 6235

Ticket

If your pull request is related to a Suricata ticket, please provide
the full URL to the ticket here so this pull request can monitor
changes to the ticket status:

Redmine ticket:

jufajardini
jufajardini requested changes Aug 22, 2023
View reviewed changes
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.

Considering the issue associated with this change, I think it would be good to also add checks for the drop reason stats counters, here, too.

@jlucovsky
Copy link
Contributor Author

Member

Good point. I've updated the tests with a stats check.

@catenacyber
Copy link
Collaborator

Why is CI green when OISF/suricata#9408 is not merged yet ?

@jufajardini
Copy link
Contributor

Why is CI green when OISF/suricata#9408 is not merged yet ?

Trying to figure that out... 🤔

@catenacyber
Copy link
Collaborator

Why is CI green when OISF/suricata#9408 is not merged yet ?

Trying to figure that out... 🤔

Because these S-V tests require feature DEBUG which is not tested by S-V CI (but I guess is tested by Suricata CI)

@jufajardini
Copy link
Contributor

Why is CI green when OISF/suricata#9408 is not merged yet ?

Trying to figure that out... 🤔

Because these S-V tests require feature DEBUG which is not tested by S-V CI (but I guess is tested by Suricata CI)

aha 🙇🏽
I must not forget this...

@jlucovsky
Copy link
Contributor Author

Why is CI green when OISF/suricata#9408 is not merged yet ?

Trying to figure that out... 🤔

Because these S-V tests require feature DEBUG which is not tested by S-V CI (but I guess is tested by Suricata CI)

aha 🙇🏽 I must not forget this...

Why is debug required? I see the requirement in the test.yaml file and would like to understand why debug is required for a core feature?

@catenacyber
Copy link
Collaborator

Debug is required for this SV test.

If it is not required for the feature, you can try to get rid of it.
Or create a new test that does not require it

@jasonish
Copy link
Member

Perhaps a README in these tests as to why debug is required?

@jlucovsky
Copy link
Contributor Author

Perhaps a README in these tests as to why debug is required?

I'll add a note -- it's because of the exception policy simulation logic the test requires.

@jufajardini
Copy link
Contributor

Sorry for adding more to this, but while doing a re-review after the push, it came to my mind: this hasn't been backported yet, right?

So, shouldn't we add min-version to the new/changed checks?

@catenacyber catenacyber added the requires suricata pr Depends on a PR in Suricata label Sep 11, 2023
@victorjulien victorjulien mentioned this pull request Sep 15, 2023
Merged
@victorjulien
Copy link
Member

Merged in #1388, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jufajardini jufajardini jufajardini requested changes

Assignees
No one assigned
Labels
requires suricata pr Depends on a PR in Suricata
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jlucovsky @catenacyber @jufajardini @jasonish @victorjulien