Hi Canh
Ack.
I see no problem with the fix. It is actually a good improvement! However I
have some comments that should be considered even if they are not directly
related to the fix as such.
Some comments:
- It's not documented what this test case is supposed to test meaning that
when reviewing I somewhat has to guess if it still does what it should do after
the fix. Please add a function comment (header comment) that explains what's
tested. This is the case for most test cases. It should be easy to fix and
could be done at least when a test case are changed or fixed for other reasons.
By doing that at least some slow improvements will happen and fixes will be
easier to review. Also if it is hard to explain what a test case is doing then
there is probably something wrong with the test case.
- When trying to analyze the test case I found that the functions that can be
found in the file utest.c does not have descriptive names, actually most of
them are misleading. For example there is a number of
<something>_validate(code, expected) functions that does not validate anything.
What they actually do is to compare some given data with some expected data
which is also given as input so they are compare functions. But they do not
report back the result of the comparing to the calling function instead the
result is reported as printed text to stdout so they could also be thought of
as report functions. This should really be fixed but could be handled in a
separate ticket.
Please fix this!
And Please, discuss with the other maintainers about:
- Constantly improve the code regarding readability for example as suggested
above.
- Never add any new "bad" fixes. For example new functions with unreadable
maybe even misleading names, test cases where it is hard to understand what's
tested, new incorrect dependencies for example fixes in wrong places such as
making "generic" functions coupled to some specific task etc.
Thanks
Lennart
> -----Original Message-----
> From: Canh Van Truong <canh.v.tru...@dektech.com.au>
> Sent: den 10 augusti 2018 09:34
> To: Lennart Lund <lennart.l...@ericsson.com>; Vu Minh Nguyen
> <vu.m.ngu...@dektech.com.au>
> Cc: opensaf-devel@lists.sourceforge.net; Canh Van Truong
> <canh.v.tru...@dektech.com.au>
> Subject: [PATCH 1/1] log: fix logtest 4 58 fail [#2909]
>
> Due to hard-disk problem, the log file has not been closed successful and
> log file has not been renamed. When previous test case (4 48) closes log file
> fail,
> test case (4 58) also count the size of (4 48)'s log file. This cause the
> size of
> log file is not 0.
>
> The patch update to check (grep) if the log record is in log file or not
> instead
> count the log file size.
> ---
> src/log/apitest/tet_LogOiOps.c | 54 +++++++++++++++-------------------------
> --
> 1 file changed, 19 insertions(+), 35 deletions(-)
>
> diff --git a/src/log/apitest/tet_LogOiOps.c b/src/log/apitest/tet_LogOiOps.c
> index 02dd20996..7aea9510b 100644
> --- a/src/log/apitest/tet_LogOiOps.c
> +++ b/src/log/apitest/tet_LogOiOps.c
> @@ -1521,9 +1521,8 @@ void verFilterOut(void)
> SaAisErrorT rc;
> char command[MAX_DATA];
> const unsigned int serverity_filter = 31;
> - FILE *fp = NULL;
> - char fileSize_c[10];
> - int fileSize = 0;
> + const char *log_record =
> + "Test filter out, log records are not writen to log file";
>
> rc = logInitialize();
> if (rc != SA_AIS_OK) {
> @@ -1545,48 +1544,33 @@ void verFilterOut(void)
> serverity_filter, SA_LOG_STREAM_APPLICATION1);
> ret = systemCall(command);
> if (ret != 0) {
> - test_validate(ret, 0);
> + rc_validate(ret, 0);
> goto done;
> }
>
> /* saflogtest sends messages */
> - sprintf(
> - command,
> - "saflogtest -a saLogApplication1 --count=2 --interval=20 --
> severity=notice"
> - " \"Test filter out, log records are not writen to log file\"");
> - rc = system(command);
> - if (rc == -1) {
> - test_validate(rc, 0);
> + sprintf(command, "saflogtest --count=2 --interval=20 --
> severity=notice "
> + "-a saLogApplication1 \"%s\"", log_record);
> + ret = systemCall(command);
> + if (ret == -1) {
> + rc_validate(ret, 0);
> goto done;
> }
>
> - /* Check if log file is empty or not*/
> + // Find the log file then check (grep) if the log record is in
> + // log file or not
> sprintf(command,
> "find %s/saflogtest -type f -mmin -1 "
> "| egrep \"%s_([0-9]{8}_[0-9]{6}\\.log$)\" "
> - "| xargs wc -c | awk '{printf $1}'",
> - log_root_path, DEFAULT_APP_FILE_NAME);
> -
> - fp = popen(command, "r");
> -
> - if (fp == NULL) {
> - /* Fail to read size of log file. Report test failed. */
> - fprintf(stderr, "Failed to run command = %s\n", command);
> - test_validate(1, 0);
> - goto done;
> - }
> - /* Get file size in chars */
> - while (fgets(fileSize_c, sizeof(fileSize_c) - 1, fp) != NULL) {
> - };
> - pclose(fp);
> -
> - /* Convert chars to number */
> - fileSize = atoi(fileSize_c);
> -
> - if (fileSize != 0) {
> - fprintf(stderr, "Log file has log records\n");
> - }
> - rc_validate(fileSize, 0);
> + "| xargs grep \"%s\" > /dev/null",
> + log_root_path, DEFAULT_APP_FILE_NAME, log_record);
> +
> + ret = system(command);
> + // Expect the log record is not found, ret != 0
> + if (ret != 0)
> + rc_validate(1, 1);
> + else
> + rc_validate(1, 0);
>
> done:
> logFinalize();
> --
> 2.15.1
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
