[syslog-ng] [PATCH (3.5) 1/3] tests: Add a testcase for log_template_compile function
Balazs Scheidler
bazsi at balabit.hu
Sat Aug 24 13:03:27 CEST 2013
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;
> +}
More information about the syslog-ng
mailing list