Suricata

Couple of more things:

- tests should clean up their memory in their success case only. Currently for many tests it's done for complicated error cases as well. All that complexity can be removed.
- FAIL_IF should be a 'return 0' in the normal case, but a wrapper to BUG_ON() in the --fatal-unittests case so we can jump right into the code line that is broken
- all conditional things in tests should go. A test should be clearly defined.

Next to this, many tests should move to a higher level. E.g. all the rule matching tests would ideally be tested for each MPM algo we support. Same for the MPM tests themselves. They are pretty much copy/pasted between the different algo implementations.

Lots of things, so probably a good idea to make this a root ticket and connect more specific ones to this. This will be a multi-step undertaking :)

Victor Julien wrote:

I'd like to also add 'PASS' and 'PASS_IF(expr)' macro's.

Its not clear to me what these would actually do?

In the current patch there are lines like this https://github.com/inliniac/suricata/pull/1984/commits/fb63e7ac1f262ce20d88786206b7f153c053835d#diff-b7c0f60a040a567a600ab22375f8b73eR4871

return ret == 0;

This could then be written as

PASS_IF(ret == 0);

Many other tests are structured such that reaching the end of the function implies 'pass'. So there instead of 'return 1;' we could simply have 'PASS;'

Victor Julien wrote:

In the current patch there are lines like this https://github.com/inliniac/suricata/pull/1984/commits/fb63e7ac1f262ce20d88786206b7f153c053835d#diff-b7c0f60a040a567a600ab22375f8b73eR4871

[...]

This could then be written as

[...]

Ahh, this was mainly to not have to fiddle with the logic of the test, yet. But yah, that could work.

Many other tests are structured such that reaching the end of the function implies 'pass'. So there instead of 'return 1;' we could simply have 'PASS;'

Just have to be sure these don't get used anywhere but at the end of a function.

I've tried a few things:

1) Moving tests into tests/ as source files and setting up autoconf in tests. Got complicated quick, couldn't test private functions.

2) Like above, but include the source being tested at the top of the test file. Would require redoing the test framework with a separate program (or multiple programs) to avoid duplicate definitions of functions.

3) Put tests in tests, and including them into the source file under test. This requires the least amount of changes. Example can be seen here: https://github.com/inliniac/suricata/compare/master...jasonish:ish-move-tests

Yeah the (3) is what I did in my stream branch work as well. I did include the header in the tests file as well though, otherwise my code highlighting thingy in vim would flag code as bad wrt unknown types and all. Perhaps there is a way around that.