[syslog-ng] [PATCH (3.5) 1/3] tests: Add a testcase for log_template_compile function
Balazs Scheidler
bazsi77 at gmail.com
Sat Aug 24 14:18:45 CEST 2013
Hi,
I got lucky, I had about an hour to work on this. This is how far I got:
https://github.com/balabit/syslog-ng-3.5/tree/log-template-refactor
I would still work on it a bit to eliminate the rest of the assert()
macros, now that ther's a cursor on the current LogTemplateElem it would
just be enough to assert on individual fields IMHO and we could get rid
off the "expected_element" local variable.
I haven't removed the defines that contain the actual testcases, but I
have promoted the names to the small test functions I have created. So I
think they should be rmoved now.
My smaller kid is sitting right next to the keyboard so I can't
continue, he insists on pressing the ESC and 'q' keys. :)
Cheers,
Bazsi
On Sat, 2013-08-24 at 13:03 +0200, Balazs Scheidler wrote:
> Hi,
>
> First of all, thanks for your contribution, especially the part that it
> makes syslog-ng core easier to grasp. I have some comments below, and
> albeit I'm being critical, please don't take it as a criticism against
> yourself, but rather the code itself.
>
> I'm also going to change the code a bit to match what I would have
> imagined just to show a _before_/_after_ comparison and make it crystal
> clear whether my approach results in code that is easier to read.
>
> Please read on below. I'm trying to refactor the test program, so I can
> post it as a reply. I have about 30 minutes before the kids wake up, so
> I have to be real quick :)
>
> If I'm not finished, we can probably talk about the changes I had in
> mind IRL when I'm in the office.
>
> Thanks.
>
> On Fri, 2013-08-23 at 18:14 +0200, Juhász Viktor wrote:
> > test_template_compile: new unit test implemented.
> >
> > Main goal of this test is to help the refactoring the log_template_compile monster function
> >
> > Signed-off-by: Juhasz Viktor <jviktor at balabit.hu>
> > ---
> > tests/unit/Makefile.am | 5 +
> > tests/unit/test_template_compile.c | 320 ++++++++++++++++++++++++++++++++++++
> > 2 files changed, 325 insertions(+)
> >
> > diff --git a/tests/unit/Makefile.am b/tests/unit/Makefile.am
> > index 8841834..af5730f 100644
> > --- a/tests/unit/Makefile.am
> > +++ b/tests/unit/Makefile.am
> > @@ -8,6 +8,7 @@ tests_unit_TESTS = \
> > tests/unit/test_serialize \
> > tests/unit/test_msgparse \
> > tests/unit/test_template \
> > + tests/unit/test_template_compile \
> > tests/unit/test_template_speed \
> > tests/unit/test_dnscache \
> > tests/unit/test_findcrlf \
> > @@ -56,6 +57,10 @@ tests_unit_test_template_LDADD = \
> > $(unit_test_extra_modules) \
> > $(PREOPEN_BASICFUNCS)
> >
> > +tests_unit_test_template_compile_CFLAGS = $(TEST_CFLAGS)
> > +tests_unit_test_template_compile_LDADD = \
> > + $(TEST_LDADD) $(unit_test_extra_modules)
> > +
> > tests_unit_test_template_speed_LDADD = \
> > $(TEST_LDADD) $(unit_test_extra_modules)
> >
> > diff --git a/tests/unit/test_template_compile.c b/tests/unit/test_template_compile.c
> > new file mode 100644
> > index 0000000..23fed4f
> > --- /dev/null
> > +++ b/tests/unit/test_template_compile.c
> > @@ -0,0 +1,320 @@
> > +#include "templates.c"
> > +#include "logmsg.h"
> > +#include "testutils.h"
> > +#include "cfg.h"
> > +#include "plugin.h"
> > +
> > +#define SIMPLE_STRING "Test String"
> > +#define SIMPLE_MACRO "${MESSAGE}"
> > +#define SIMPLE_MACRO_UNBRACE "$MESSAGE"
> > +#define SIMPLE_MACRO_UNBRACE_ADD_TEXT "$MESSAGE test value"
> > +#define SIMPLE_MACRO_MSGREF "${MESSAGE}@1"
> > +#define SIMPLE_MACRO_INVALID_REF "${MESSAGE}@gmail.com"
> > +#define SIMPLE_MACRO_ADD_TEXT "${MESSAGE}test value"
> > +#define ESCAPED_CHAR "Test \\$STRING"
> > +#define VALID_SUBST "${MESSAGE:-default value}"
> > +#define TRICKY_VALUE "$$VALUE_NAME"
> > +#define TRICKY_VALUE_2 "$:VALUE_NAME"
> > +#define TRICKY_VALUE_3 "$${VALUE_NAME}"
> > +#define TRICKY_VALUE_4 "\\"
> > +#define TRICKY_VALUE_5 "$"
> > +#define TRICKY_VALUE_6 "${MESSAGE:-}"
> > +#define TRICKY_VALUE_7 "${}"
> > +
> > +#define INVALID_MACRO "${MESSAGE"
> > +#define INVALID_SUBST "${MESSAGE:1}"
> > +
> > +#define SIMPLE_VALUE "${VALUE_NAME}"
> > +#define SIMPLE_VALUE_UNBRACE "$VALUE_NAME"
> > +#define SIMPLE_VALUE_ESCAPED "${VALUE\\}NAME}"
> > +
> > +#define SIMPLE_TEMPLATE_FUNCTION "$(hello)"
> > +#define SIMPLE_TEMPLATE_FUNCTION_WITH_ADDITIONAL_TEXT "$(hello)test value"
> > +#define COMPLICATED_TEMPLATE_FUNCTION "$( hello \\tes\t\t\t value(xyz) \"value with spaces\" 'test value with spa\"ces')@2"
> > +#define COMPLICATED_TEMPLATE_FUNCTION_BAD_1 "$( hello \\tes\t\t\t value(xyz \"value with spaces\" 'test value with spa\"ces')"
> > +#define COMPLICATED_TEMPLATE_FUNCTION_BAD_2 "$( hello \\tes\t\t\t value xyz \"value with spaces\" 'test value with spa\"ces'"
> > +
> > +#define SIMPLE_UNKNOWN_FUNCTION "$(unknown function)"
>
> Please don't define testcases in advance, especially if you can't give
> them names that _really_ describes their purpose.
>
> Defining TRICKY_VALUE3 and using it later on in the code absolutely
> increases the need to look up the definition while reading the code.
> These are distractions while trying to understand code.
>
> Whenever you implement a testcase, name the testcase function based on
> the _functional_ intention that you want to test. Make helper
> functions/macros so that understanding the testcase itself is trivial,
> without having to look sideways.
>
> > +
> > +static void
> > +hello(LogMessage *msg, int argc, GString *argv[], GString *result)
> > +{
> > + return;
> > +}
> > +
> > +TEMPLATE_FUNCTION_SIMPLE(hello);
> > +Plugin hello_plugin = TEMPLATE_FUNCTION_PLUGIN(hello, "hello");
> > +
> > +
> > +GlobalConfig *configuration;
> > +static void
> > +hide_internal_message(LogMessage *msg)
> > +{
> > + log_msg_unref(msg);
> > + return;
> > +}
>
> hmm... there are asserts implemented on internal messages within
> testutils right now. that might be better fit for this purpose.
>
> void reset_grabbed_messages(void);
> void start_grabbing_messages(void);
> void stop_grabbing_messages(void);
> gboolean assert_grabbed_messages_contain_non_fatal(const gchar *pattern, const gchar *error_message, ...);
>
> #define assert_grabbed_messages_contain(pattern, error_message, ...) (assert_grabbed_messages_contain_non_fatal(pattern, error_message, ##__VA_ARGS__) ? 1 : (exit(1),0))
>
>
>
> > +
> > +#define assert_common_element(actual, expected) \
> > + assert_string(actual.text, expected.text, ASSERTION_ERROR("Bad compiled template text")); \
> > + if (expected.default_value) \
> > + { \
> > + assert_string(actual.default_value, expected.default_value, ASSERTION_ERROR("Bad compiled template default value")); \
> > + } \
> > + else \
> > + { \
> > + assert_gpointer(actual.default_value, NULL, ASSERTION_ERROR("Bad compiled template default value")); \
> > + } \
> > + assert_gint(actual.msg_ref, expected.msg_ref, ASSERTION_ERROR("Bad compiled template msg_ref"));
> > +
> > +#define assert_value_element(actual, expected) \
> > + assert_gint(actual.type, LTE_VALUE, ASSERTION_ERROR("Bad compiled template type")); \
> > + assert_common_element(actual, expected); \
> > + assert_gint(actual.value_handle, expected.value_handle, ASSERTION_ERROR("Bad compiled template macro"));
> > +
> > +#define assert_macro_element(actual, expected) \
> > + assert_gint(actual.type, LTE_MACRO, ASSERTION_ERROR("Bad compiled template type")); \
> > + assert_common_element(actual, expected); \
> > + assert_gint(actual.macro, expected.macro, ASSERTION_ERROR("Bad compiled template macro"));
> > +
> > +#define assert_func_element(actual, expected) \
> > + assert_gint(actual.type, LTE_FUNC, ASSERTION_ERROR("Bad compiled template type")); \
> > + assert_common_element(actual, expected); \
> > + assert_gpointer(actual.func.ops, expected.func.ops, ASSERTION_ERROR("Bad compiled template macro"));
> > +
> > +#define assert_bad_element(actual, expected, expected_error_message) \
> > + assert_string(err->message, expected_error_message, ASSERTION_ERROR("Bad error message")); \
> > + assert_macro_element(actual, expected)
> > +
> > +
> > +#define assert_template_compile(template_string) \
> > + assert_true(log_template_compile(template, template_string, &err), ASSERTION_ERROR("Can't compile template")); \
> > + assert_string(template->template, template_string, ASSERTION_ERROR("Bad stored template"));
> > +
> > +#define assert_failed_template_compile(template_string) assert_false(log_template_compile(template, template_string, &err), ASSERTION_ERROR("Can compile bad template"));
> > +
> > +#define fill_expected_template_element(element, text, default_value, spec, type, msg_ref) {\
> > + element.text; \
> > + element.default_value; \
> > + element.spec; \
> > + element.type; \
> > + element.msg_ref; }
> > +
> > +
> > +
> > +void
> > +test_template_compile_macro()
>
> Please try to name your test functions in a way that explains the
> intent. Does this test template strings that contain only a single macro
> element or?
>
> > +{
> > + GError *err = NULL;
> > + LogTemplateElem *element;
> > + LogTemplateElem expected_element;
> > + LogTemplate *template = log_template_new(configuration, NULL);
> > +
> > + assert_template_compile(SIMPLE_STRING);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = SIMPLE_STRING, default_value = NULL, macro = M_NONE, type = LTE_MACRO, msg_ref = 0);
> > + assert_macro_element(element[0], expected_element);
>
> hmm... this relies on LogTemplate internal representation and is not in
> any way encapsulated into a function, but rather copy-pasted in all the
> testcases. not really good this way.
>
> > +
> > + assert_template_compile(SIMPLE_MACRO);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "", default_value = NULL, macro = M_MESSAGE, type = LTE_MACRO, msg_ref = 0);
> > + assert_macro_element(element[0], expected_element);
> > +
> > + assert_template_compile(SIMPLE_MACRO_ADD_TEXT);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "", default_value = NULL, macro = M_MESSAGE, type = LTE_MACRO, msg_ref = 0);
> > + assert_macro_element(element[0], expected_element);
> > + element = (LogTemplateElem *)template->compiled_template->next->data;
> > + fill_expected_template_element(expected_element, text = "test value", default_value = NULL, macro = M_NONE, type = LTE_MACRO, msg_ref = 0);
> > + assert_macro_element(element[0], expected_element);
> > +
> > + assert_template_compile(SIMPLE_MACRO_UNBRACE);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "", default_value = NULL, macro = M_MESSAGE, type = LTE_MACRO, msg_ref = 0);
> > + assert_macro_element(element[0], expected_element);
> > +
> > + assert_template_compile(SIMPLE_MACRO_UNBRACE_ADD_TEXT);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "", default_value = NULL, macro = M_MESSAGE, type = LTE_MACRO, msg_ref = 0);
> > + assert_macro_element(element[0], expected_element);
> > + element = (LogTemplateElem *)template->compiled_template->next->data;
> > + fill_expected_template_element(expected_element, text = " test value", default_value = NULL, macro = M_NONE, type = LTE_MACRO, msg_ref = 0);
> > + assert_macro_element(element[0], expected_element);
> > +
> > + assert_template_compile(SIMPLE_MACRO_MSGREF);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "", default_value = NULL, macro = M_MESSAGE, type = LTE_MACRO, msg_ref = 2);
> > + assert_macro_element(element[0], expected_element);
> > +
> > + assert_template_compile(SIMPLE_MACRO_INVALID_REF);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "", default_value = NULL, macro = M_MESSAGE, type = LTE_MACRO, msg_ref = 1);
> > + assert_macro_element(element[0], expected_element);
> > + element = (LogTemplateElem *)template->compiled_template->next->data;
> > + fill_expected_template_element(expected_element, text = "gmail.com", default_value = NULL, macro = M_NONE, type = LTE_MACRO, msg_ref = 0);
> > + assert_macro_element(element[0], expected_element);
> > +
> > + assert_template_compile(ESCAPED_CHAR);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "Test $STRING", default_value = NULL, macro = M_NONE, type = LTE_MACRO, msg_ref = 0);
> > + assert_macro_element(element[0], expected_element);
> > +
> > + assert_template_compile(VALID_SUBST);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "", default_value = "default value", macro = M_MESSAGE, type = LTE_MACRO, msg_ref = 0);
> > + assert_macro_element(element[0], expected_element);
> > +
> > + assert_template_compile(TRICKY_VALUE);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "$VALUE_NAME", default_value = NULL, macro = M_NONE, type = LTE_MACRO, msg_ref = 0);
> > + assert_macro_element(element[0], expected_element);
> > +
> > + assert_template_compile(TRICKY_VALUE_2);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "$:VALUE_NAME", default_value = NULL, macro = M_NONE, type = LTE_MACRO, msg_ref = 0);
> > + assert_macro_element(element[0], expected_element);
> > +
> > + assert_template_compile(TRICKY_VALUE_3);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "${VALUE_NAME}", default_value = NULL, macro = M_NONE, type = LTE_MACRO, msg_ref = 0);
> > + assert_macro_element(element[0], expected_element);
> > +
> > + assert_template_compile(TRICKY_VALUE_4);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "", default_value = NULL, macro = M_NONE, type = LTE_MACRO, msg_ref = 0);
> > + assert_macro_element(element[0], expected_element);
> > +
> > + assert_template_compile(TRICKY_VALUE_5);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "$", default_value = NULL, macro = M_NONE, type = LTE_MACRO, msg_ref = 0);
> > + assert_macro_element(element[0], expected_element);
> > +
> > + assert_template_compile(TRICKY_VALUE_6);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "", default_value = "", macro = M_MESSAGE, type = LTE_MACRO, msg_ref = 0);
> > + assert_macro_element(element[0], expected_element);
> > +
> > + log_template_unref(template);
> > +}
>
> lots of copy/paste and difficult to understand the individual testcases
> because the string values are elsewhere.
>
> I don't know what the last one tries to test. I could certainly make
> more effort to understand it, but the point of unit tests is to convey
> concise information about the requirements the code wanted to meet.
>
> > +
> > +void
> > +test_template_compile_value()
> > +{
> > + GError *err = NULL;
> > + LogTemplateElem *element;
> > + LogTemplateElem expected_element;
> > + LogTemplate *template = log_template_new(configuration, NULL);
> > +
> > + assert_template_compile(SIMPLE_VALUE);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "", default_value = NULL, value_handle = log_msg_get_value_handle("VALUE_NAME"), type = LTE_VALUE, msg_ref = 0);
> > + assert_value_element(element[0], expected_element);
> > +
> > + assert_template_compile(SIMPLE_VALUE_UNBRACE);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "", default_value = NULL, value_handle = log_msg_get_value_handle("VALUE_NAME"), type = LTE_VALUE, msg_ref = 0);
> > + assert_value_element(element[0], expected_element);
> > +
> > + assert_template_compile(SIMPLE_VALUE_ESCAPED);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "", default_value = NULL, value_handle = log_msg_get_value_handle("VALUE\\"), type = LTE_VALUE, msg_ref = 0);
> > + assert_value_element(element[0], expected_element);
> > +
> > + assert_template_compile(TRICKY_VALUE_7);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "", default_value = NULL, value_handle = log_msg_get_value_handle(""), type = LTE_VALUE, msg_ref = 0);
> > + assert_value_element(element[0], expected_element);
> > +
> > + log_template_unref(template);
> > +}
> > +
> > +void
> > +test_template_compile_func()
> > +{
> > + GError *err = NULL;
> > + LogTemplateElem *element;
> > + LogTemplateElem expected_element;
> > + LogTemplate *template = log_template_new(configuration, NULL);
> > + plugin_register(configuration, &hello_plugin, 1);
> > +
> > + assert_template_compile(SIMPLE_TEMPLATE_FUNCTION);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "", default_value = NULL, func.ops = hello_construct(&hello_plugin, configuration, LL_CONTEXT_TEMPLATE_FUNC, "hello"), type = LTE_FUNC, msg_ref = 0);
> > + assert_func_element(element[0], expected_element);
> > +
> > + assert_template_compile(COMPLICATED_TEMPLATE_FUNCTION);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "", default_value = NULL, func.ops = hello_construct(&hello_plugin, configuration, LL_CONTEXT_TEMPLATE_FUNC, "hello"), type = LTE_FUNC, msg_ref = 3);
> > + assert_func_element(element[0], expected_element);
> > +
> > + assert_template_compile(SIMPLE_TEMPLATE_FUNCTION_WITH_ADDITIONAL_TEXT);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "", default_value = NULL, func.ops = hello_construct(&hello_plugin, configuration, LL_CONTEXT_TEMPLATE_FUNC, "hello"), type = LTE_FUNC, msg_ref = 0);
> > + assert_func_element(element[0], expected_element);
> > + element = (LogTemplateElem *)template->compiled_template->next->data;
> > + fill_expected_template_element(expected_element, text = "test value", default_value = NULL, macro = M_NONE, type = LTE_MACRO, msg_ref = 0);
> > + assert_macro_element(element[0], expected_element);
> > +
> > + log_template_unref(template);
> > +}
> > +
> > +void
> > +test_template_compile_negativ_tests()
> > +{
> > + GError *err = NULL;
> > + LogTemplateElem *element;
> > + LogTemplateElem expected_element;
> > + LogTemplate *template = log_template_new(configuration, NULL);
> > +
> > + assert_failed_template_compile(INVALID_MACRO);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "error in template: ${MESSAGE", default_value = NULL, macro = M_NONE, type = LTE_MACRO, msg_ref = 0);
> > + assert_bad_element(element[0], expected_element, "Invalid macro, '}' is missing, error_pos='9'");
> > + g_clear_error(&err);
> > +
> > +
> > + assert_failed_template_compile(INVALID_SUBST);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "error in template: ${MESSAGE:1}", default_value = NULL, macro = M_NONE, type = LTE_MACRO, msg_ref = 0);
> > + assert_bad_element(element[0], expected_element, "Unknown substitution function, error_pos='10'");
> > + g_clear_error(&err);
> > +
> > +
> > + assert_failed_template_compile(COMPLICATED_TEMPLATE_FUNCTION_BAD_1);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "error in template: $( hello \\tes\t\t\t value(xyz \"value with spaces\" 'test value with spa\"ces')", default_value = NULL, macro = M_NONE, type = LTE_MACRO, msg_ref = 0);
> > + assert_bad_element(element[0], expected_element, "Invalid template function reference, missing function name or inbalanced '(', error_pos='73'");
> > + g_clear_error(&err);
> > +
> > + assert_failed_template_compile(COMPLICATED_TEMPLATE_FUNCTION_BAD_2);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "error in template: $( hello \\tes\t\t\t value xyz \"value with spaces\" 'test value with spa\"ces'", default_value = NULL, macro = M_NONE, type = LTE_MACRO, msg_ref = 0);
> > + assert_bad_element(element[0], expected_element, "Invalid template function reference, missing function name or inbalanced '(', error_pos='72'");
> > + g_clear_error(&err);
> > +
> > + assert_failed_template_compile(SIMPLE_UNKNOWN_FUNCTION);
> > + element = (LogTemplateElem *)template->compiled_template->data;
> > + fill_expected_template_element(expected_element, text = "error in template: $(unknown function)", default_value = NULL, macro = M_NONE, type = LTE_MACRO, msg_ref = 0);
> > + assert_bad_element(element[0], expected_element, "Unknown template function unknown");
> > + g_clear_error(&err);
> > +
> > + log_template_unref(template);
> > +}
>
> Same issues in all the testcases. I'd try to do this in a different way.
>
> > +
> > +int main(int argc, char **argv)
> > +{
> > + msg_init(FALSE);
> > + configuration = cfg_new(0x305);
> > + log_msg_registry_init();
> > + log_template_global_init();
> > +
> > + msg_set_post_func(hide_internal_message);
> > +
> > + test_template_compile_macro();
> > + test_template_compile_value();
> > + test_template_compile_func();
> > + test_template_compile_negativ_tests();
> > +
> > + log_msg_registry_deinit();
> > + msg_deinit();
> > + return 0;
> > +}
>
>
> ______________________________________________________________________________
> Member info: https://lists.balabit.hu/mailman/listinfo/syslog-ng
> Documentation: http://www.balabit.com/support/documentation/?product=syslog-ng
> FAQ: http://www.balabit.com/wiki/syslog-ng-faq
>
More information about the syslog-ng
mailing list